AllenNeuralDynamics / AllenNeuralDynamics/ficus
Notes on ficus design
- Vorherrschende Sprache
- Python
- Sterne
- 1
- Forks
- 0
- PR-Merge-Kennzahlen
- Keine gemergten PRs in 30 T.
Beschreibung
### API around scope/identifier is not consistent.
Scope and identifier should remain separate concepts. Scope is a validated dimension defined by the server, identifier is an arbitrary value within it, and collapsing them loses that distinction. However in the /configs router point, this distinction is blurred by encoding it as yet another concept: scope_identifier:
```python
def _parse_scope_identifiers(
scope_identifiers: list[str] = Query(default=[]),
) -> dict[str, str]:
"""Parse repeated ``scope_identifiers=key:value`` query params into a dict.
Example: ``?scope_identifiers=env:prod&scope_identifiers=region:us-east``
"""
result = {}
for item in scope_identifiers:
if ":" not in item:
raise HTTPException(
status_code=422,
detail=f"Invalid scope_identifier '{item}': expected format 'key:value'",
)
k, v = item.split(":", 1)
result[k] = v
return result
```
Consider instead keeping the distinction:
```python
def _parse_scope_identifiers(
scope: list[str] = Query(default=[]),
scope_identifier: list[str] = Query(default=[]),
) -> dict[str, str]:
if len(scope) != len(scope_identifier):
raise HTTPException(status_code=422, detail="scope and scope_identifier must be paired")
return dict(zip(scope, scope_identifier))
```
and called as:
`?scope=hostname&scope_identifier=rig-01&scope=subject_id&scope_identifier=mouse-42` similar to what the /leaves router already does.
Having two syntaxes for it means two parsing paths, two sets of validation errors, and double the surface area for client mistakes. A leaf additionally enforces only one scope at a time but uses different param names. One interface is enough.
### Rename `mode`
`Mode` is a super generic parameter mode that does not provide much information about how the argument is intended to be used at all. Consider renaming it "profile" or "preset" instead
### `ConfigObject` leals database details
Both `ConfigObject` and `override_stack` (`get_override_stack`) work on top of "paths". This is a leaky abstraction that reveals the underlying architecture of the database. Since you already have the proper abstraction, why not continue with it?
```python
# current
"override_stack": ["/scratch/defaults/my-ns/default.yml",
"/scratch/hostname/rig-01/my-ns/default.yml"]
# proposed (note you can also use a small record-like class)
"override_stack": [{"scope": "defaults", "identifier": null, "mode": "default"},
{"scope": "hostname", "identifier": "rig-01", "mode": "default"}]
```
### suffix is unnecessary?
Making clients specify `.yml` vs `.json` is an implementation detail that bleeds into the request contract. On read, the format is detected automatically. On write, the client must remember to pass `suffix=.yml` or the default applies. This asymmetry is confusing. Consider accepting a `format` query param (`yaml` / `json`) with a clear default, or using the request `Content-Type` header to negotiate format, keeping `suffix` as an internal concern.
### `_deep_update_existing_destructive` mutates its argument
Make the drain explicit:
```python
def _deep_update_returning_remainder(dest: dict, source: dict) -> tuple[dict, dict]:
dest = copy.deepcopy(dest)
source = copy.deepcopy(source)
for key in list(source.keys()):
if key in dest:
if isinstance(dest[key], dict) and isinstance(source[key], dict):
dest[key], source[key] = _deep_update_returning_remainder(dest[key], source[key])
if not source[key]:
source.pop(key)
else:
dest[key] = source.pop(key)
return dest, source
```
The loop inside `save_config` becomes:
```python
data_cpy = copy.deepcopy(data)
for idx, filepath in reversed(list(enumerate(override_stack_new_suffix))):
...
level_cfg, data_cpy = _deep_update_returning_remainder(level_cfg, data_cpy)
```
data_cpy is visibly reassigned on every iteration. The drain is part of the function contract, not a hidden consequence of calling it.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Bewertung
Dieses Issue wurde noch nicht bewertet.