AllenNeuralDynamics / AllenNeuralDynamics/ficus

Notes on ficus design

未关闭
#81 1 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
Python
星标
1
派生
0
PR 合并指标
30 天内没有已合并 PR

描述

### 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.

贡献指南

这个仓库没有索引到贡献指南

评估

这个 Issue 还没有评估数据。

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。