AllenNeuralDynamics / AllenNeuralDynamics/ficus

Notes on ficus design

Đang mở
#81 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
1
Fork
0
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

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

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.