[Discussion] How to Improve the Readability and Composability of the NativeLink Config JSON part 1 #834
Comments
|
I like this. It reminds me of how such configs would look like in YAML. Some ideas (which effectively can be compiled down to json): GHA style (seems nice and concise initially, but the more I think about it the less convinced I get as it seems very easy to mess up when configs become more complex): ---
stores:
- name: ac-main-store-fs
uses: filesystem-store
with:
contentPath: /tmp/nativelink/data-worker-test/content_path-ac
tempPath: /tmp/nativelink/data-worker-test/tmp_path-ac
- name: ac-main-store-memory
uses: memory-store
with:
maxSize: 20G
- name: ac-main-store
uses: fast-slow-store
with:
fast: ac-main-store-memory
slow: ac-main-store-fsK8s style (seems overly verbose initially, but the more I think about it the more I like it as it provides a clear, familiar structure and a standardized way to iterate on the API): ---
apiVersion: nativelink/v1alpha1
kind: FilesystemStore
metadata:
name: ac-main-store-fs
spec:
contentPath: /tmp/nativelink/data-worker-test/content_path-ac
tempPath: /tmp/nativelink/data-worker-test/tmp_path-ac
---
apiVersion: nativelink/v1alpha1
kind: MemoryStore
metadata:
name: ac-main-store-memory
spec:
maxSize: 20G
---
apiVersion: nativelink/v1alpha1
kind: FastSlowStore
metadata:
name: ac-main-store
spec:
fast: ac-main-store-memory
slow: ac-main-store-fs |
|
I thought about this for a long time and the only possible downside of the new format is that lookup in the vector format (example 2) changes from direct access to O(n) time. However, given that we will never have more than 5 or 10 stores in NativeLink, I don't see this as a material concern. In the worst case scenario, 100 stores would still not cause any negligible performance problems. |
|
In this case the Big-O complexity doesn't change. We have to translate these configs into the implementation which requires iterating through them all at startup time. Internally it'll still hold them as a I'm fine with this, the only bikeshed I have is |
|
I'd like to work on this issue. |
|
I've questions while I was working on this issue. Before: After: Do we need this change in this PR? cc: @MarcusSorealheis , @aaronmondal , @allada , @adam-singer |
|
Okay, I already made PR for both |
To migrate, change your config like this (similar for schedulers):
```
// Old:
"stores": {
"SOMESTORE": {
"memory": {}
}
}
// New:
"stores": [
{
"name": "SOMESTORE",
"memory": {}
}
]
```
Closes TraceMachina#834
To migrate, change your config like this (similar for schedulers):
```
// Old:
"stores": {
"SOMESTORE": {
"memory": {}
}
}
// New:
"stores": [
{
"name": "SOMESTORE",
"memory": {}
}
]
```
Closes TraceMachina#834
To migrate, change your config like this (similar for schedulers):
```
// Old:
"stores": {
"SOMESTORE": {
"memory": {}
}
}
// New:
"stores": [
{
"name": "SOMESTORE",
"memory": {}
}
]
```
Closes TraceMachina#834
To migrate, change your config like this (similar for schedulers):
```
// Old:
"stores": {
"SOMESTORE": {
"memory": {}
}
}
// New:
"stores": [
{
"name": "SOMESTORE",
"memory": {}
}
]
```
For now this is not a breaking change, but a backwards compatible one.
The backwards-compatibility code will be removed after the next release
in a breaking change.
Closes TraceMachina#834
To migrate, change your config like this (similar for schedulers):
```
// Old:
"stores": {
"SOMESTORE": {
"memory": {}
}
}
// New:
"stores": [
{
"name": "SOMESTORE",
"memory": {}
}
]
```
For now this is not a breaking change, but a backwards compatible one.
The backwards-compatibility code will be removed after the next release
in a breaking change.
Closes TraceMachina#834
Currently, one area of confusion stems from the fact that we accept user-defined keys (in other words, dynamic keys) for the names of stores and other entities. It's fine to have user-defined names, but they should be anchored to a non-dynamic key, and added as fields. Here is an example config in our current state:
"AC_MAIN_STORE" and "FS_CONTENT_STORE" are both user-supplied values.
Below is an improved version of the same config where:
the name is dynamic but the key is always name
the stores objects are nested in an array because multiple objects should be in an array
the details about each store are embedded in a self-describing key called
options.The sum of all these changes results in many improvements.
The text was updated successfully, but these errors were encountered: