Repository navigation
Feat: registry (backends / transports / schedulers) - #28
Alexander-Auer wants to merge 44 commits into
Conversation
… have them link to each other
…ede into feat/core-contract
…a consumer should choose to use it or not
* feat: Models the core contract needs * Feat: Protocols and Exceptions added for the core contract * Feat: Private protocols for the BatchBackend * feat: a fake backend to run unit tests against * fix: some code cleanup in the helper functions * feat: setup for the unit test suite * chore: style fix * chore: styling, unit test fixture * chore: single letter var `h` refering to hanlde changed to `handle` * fix: unused parameter in `test_injected_failure_is_reported_per_job()` removed * feat: Models the core contract needs * Feat: Protocols and Exceptions added for the core contract * Feat: Private protocols for the BatchBackend * feat: a fake backend to run unit tests against * fix: some code cleanup in the helper functions * feat: setup for the unit test suite * chore: style fix * chore: styling, unit test fixture * chore: single letter var `h` refering to hanlde changed to `handle` * fix: unused parameter in `test_injected_failure_is_reported_per_job()` removed * fix: made the OutputSpec optional as it is not suppoerted by all and a consumer should choose to use it or not --------- Co-authored-by: alexander <alexander.auer@cern.ch>
…d of a hash of the full object
Add BackendConfig, one flat public model a consumer uses to describe any backend (kind, transport, scheduler, endpoint, host, ...). It is resolved internally into the settings of exactly one backend family, so a field that does not apply to the chosen kind is refused by name instead of ignored. Add the registry: registry.backend(config) builds the backend a configuration describes, from closed name -> "module:Class" tables. Entries are imported lazily and every built object is checked against its protocol, so a broken entry fails at resolution with a RegistryError. Part of DIRACGrid#18. Co-authored-by: alexander <alexander.auer@cern.ch>
Drop tests of pydantic and Python behaviour (immutability, defaults, hashing) and keep the rules interCEde itself adds.
One test per rule, no tests of what pydantic or a closed table already guarantees.
Move the API reference to docs/user/reference/api.md, link the user and developer documentation from the index, and stop referring to the ADRs there.
- Google-style docstrings for the public and batch protocols, with arguments, returns and raises. - get_diagnostics takes a filename (a file in the job's working directory; None reads stdout) instead of a generic name; Diagnostics.filename matches. - An input name may appear once in a SubmissionSpec: two sources for one file are refused when the spec is built. - A copy with a secret named like an input is refused at submit with SpecificationRejectedError; SubmissionSpec.clashes_with() lets every backend apply the same rule. - No references to the ADRs in code or docstrings: they are not part of the source and can be superseded.
Follows the review of the core contract: the ADRs are not part of the source and can be superseded. The registry's docstrings also say what they return.
- Google-style docstrings with Args, Returns and Raises for resolve() and every registry and configuration helper, and docstrings for the settings classes; ruff now checks docstrings against the Google convention. - Remove vm_max_lifetime_s: removing VMs after a lifetime is the consumer's policy, not interCEde's. - Import Callable from collections.abc, like the rest of the code.
aldbr
left a comment
There was a problem hiding this comment.
A few comments, most are minor.
I just have a question around BackendConfig: I would lean towards different public interfaces rather than 1 big interface. But no strong opinion about it, we can discuss
| } | ||
|
|
||
|
|
||
| def backend(config: BackendConfig | Mapping[str, Any]) -> JobBackend: |
There was a problem hiding this comment.
What about naming it get_backend()?
There was a problem hiding this comment.
that is probably a bit nicer yes
| try: | ||
| config = BackendConfig.model_validate(config) | ||
| except ValidationError as exc: | ||
| raise ConfigurationError(str(exc)) from exc |
There was a problem hiding this comment.
Is there any risk of leaking secrets?
I know it shouldn't be the case, but one could potentially misunderstand the API and try to add a secret
Example:
from intercede import registry
config = read_from_file()
# {"kind": "batch", ..., "password": "toto4"} ⚠️
async with registry.backend(config) as ce:
await ce.submit(spec, copies)1 validation error for BackendConfig
password
Extra inputs are not permitted [type=extra_forbidden, input_value='toto4', input_type=str]What do you think?
Can you check if there is any easy way of just displaying expected values (declared in BackendConfig and scalar)?
| RegistryError: The configuration is valid, but this version of | ||
| interCEde does not provide what it asks for. | ||
| """ | ||
| if not isinstance(config, BackendConfig): |
There was a problem hiding this comment.
Can you see whether this raises an error please?
base = BackendConfig(
kind="batch",
transport="ssh",
scheduler="slurm",
host="login.example.org",
known_hosts="login.example.org ssh-ed25519 AAAA…"
)
local = base.model_copy(update={"transport": "local"}) # forgot to clear host/known_hosts
registry.backend(local)| ============================ ========================================== | ||
| ``kind`` fields | ||
| ============================ ========================================== | ||
| ``arc``, ``htcondor-ce`` ``endpoint``, ``auth`` (``token``, ``proxy``) | ||
| ``cloud`` ``endpoint``, ``driver``, ``vm_image``, | ||
| ``vm_flavor``, ``driver_options``, | ||
| ``vm_networks``, ``vm_ssh_key`` | ||
| ``batch`` + ``local`` ``scheduler`` | ||
| ``batch`` + ``ssh`` ``scheduler``, ``host``, ``user``, | ||
| ``known_hosts``, ``port``, ``jump_host``, | ||
| ``connect_timeout_s``, ``auth`` (one of | ||
| ``key``, ``password``, ``agent``) | ||
| all but ``cloud`` ``queue``, ``container_launcher``, | ||
| ``max_output_bytes``, ``max_output_files``, | ||
| ``transfer_timeout_s`` | ||
| ============================ ========================================== |
There was a problem hiding this comment.
This has the potential of creating discrepancies if any of these fields is updated.
It would be probably be useful but would be better if it's automatically generated from the code somewhere in the documentation for instance.
See https://github.com/DIRACGrid/diracx/blob/3223011c5b8cc1d0a77636470ac40e76c19e9e36/pixi.toml#L246 in diracx for instance
| from pydantic import ValidationError | ||
|
|
||
| from intercede import BackendConfig | ||
| from intercede._backends.config import ( |
There was a problem hiding this comment.
Note: ideally, we want unit tests to be focused on the public API to avoid changing the unit tests for a minor change in a private function.
Unit tests on private components can be interesting if something is complex to test though
| LIMIT_FIELDS = ("max_output_bytes", "max_output_files", "transfer_timeout_s") | ||
| SSH_FIELDS = ("host", "port", "user", "known_hosts", "jump_host", "connect_timeout_s") |
There was a problem hiding this comment.
These fields are defined twice (here and in the pydantic models): this can introduce discrepancies.
Ideally you get them from the models like:
LIMIT_FIELDS = tuple(Limits.model_fields)
SSH_FIELDS = tuple(name for name in SSHTransportSettings.model_fields if name not in ("kind", "auth"))| _SECRET_NAMES = ("secret", "password", "token", "passphrase") | ||
| _SECRET_EXACT = ("key", "api_key", "access_key", "private_key") |
There was a problem hiding this comment.
Out of curiosity, did you choose these names yourself or is it based on any guidelines?
| try: | ||
| config = BackendConfig.model_validate(config) | ||
| except ValidationError as exc: | ||
| raise ConfigurationError(str(exc)) from exc | ||
| return _build(resolve(config)) |
There was a problem hiding this comment.
Here resolve() is called twice: 1st by _fits_the_backend calling resolve() and then here. Is that expected?
| __all__ = ["BackendConfig"] | ||
|
|
||
|
|
||
| class BackendConfig(_Model): |
There was a problem hiding this comment.
Do we need BackendConfig?
Sounds like a potential source of discrepancy here also.
What about:
BackendConfig = Annotated[
Union[
Annotated[ARCConfig, Tag("arc")],
Annotated[CloudConfig, Tag("cloud")],
Annotated[BatchLocalConfig, Tag("batch+local")],
Annotated[BatchSSHConfig, Tag("batch+ssh")]
],
Discriminator(lambda v: f"batch+{v.get('transport')}" if v.get("kind") == "batch" else v.get("kind")),
]Where ...Config would correspond to your ...Settings classes.
The user would get:
- Generated reference docs: one page per backend with exactly its fields (the current vesion would give one page of ~25 optional fields).
- Typed code
- If we ever publish JSON schema, editors could suggest only the fields valid for the chosen kind.
But the flat model is also convenient:
- One interface, and internal classes can be reorganised without breaking the API.
- Load errors read slightly better today:
host: does not apply to kind='arc'vsarc.host: Extra inputs are not permitted.
Do you have any opinion?
feat: backend configuration and a closed registry
Part of #18.
Important
Depends on #27. Merge after #27 (PR 2 of 4; the full order is listed in #27).
Until #27 is merged, this diff also shows the earlier PRs. This PR's own change is one commit:
f90cdd4A consumer can now say which backend it wants as plain data, and get a backend for it:
Configuration: one flat public model
BackendConfigis the only public configuration class. The consumer uses the same field names for every backend, and never needs to know which classes interCEde uses inside:kindarc,htcondor-ceendpoint,auth(token,proxy)cloudendpoint,driver,vm_image,vm_flavor,driver_options,vm_networks,vm_ssh_key,vm_max_lifetime_sbatch+localschedulerbatch+sshscheduler,host,user,known_hosts,port,jump_host,connect_timeout_s,auth(key,passwordoragent)cloudqueue,container_launcher,max_output_bytes,max_output_files,transfer_timeout_sThe three backend families of IC-ADR-001 §4 are covered:
arc,htcondor-cecloudbatch, a transport combined with a schedulerHow it is checked. Internally,
resolve()turns the flat model into the settings of exactly one backend (_backends/config.py), and the model calls it while it is being built. So mistakes are reported immediately, in the consumer's own field names:Rules:
driver_optionsadditionally refuses secret-looking keys.known_hosts, because host key verification is not optional.container_launchermust contain an{image}placeholder.Registry: closed, lazy, checked
_BACKENDS,_TRANSPORTSand_SCHEDULERSmap a name to"module:Class"and live in interCEde itself, as dictionaries rather than entry points, so nothing outside the library can add to them (IC-ADR-001 Rejected Ideas).import intercedeimports no backend.from_settings(...). The registry builds the object and checks it withisinstanceagainstJobBackend,TransportorScheduler. A missing module, a missing class, a missingfrom_settings, or an object that breaks its protocol is reported as aRegistryErrorat resolution, never at the first call.batch, the transport is built from its own settings and the scheduler from the batch settings, then both are passed to the batch backend. Sossh+slurmandlocal+slurmshare one class.registry.available()lists what a configuration can ask for in this version, for tooling.Errors.
ConfigurationErrormeans the configuration is invalid. The newRegistryErrormeans the configuration is valid, but this version does not ship what it asks for, or an entry is broken.The tables currently contain only the internal fake, which the conformance suite now builds through the registry. Each real backend from issues #19 to #23 adds one line plus a
from_settings. Until then, valid real configurations correctly report, for example,backend 'arc' is not available in this version of interCEde.Differences from issue #18 and the ADR
{"kind": "batch", "transport": "ssh", "scheduler": "slurm", "host": ...}) and keeps the per-family settings internal.lsf,sge,oarandtorqueare left out for now. Adding a kind later is non-breaking.Tests
tests/unit/test_config.py: every family, required and refused fields, auth values, limits, and the error messagestests/unit/test_registry.py: resolution per family, composition, broken entries, caching,available(), lazy imports, and that calls on the returned object reach the registered classtests/unit/_registry_dummies.py: stand-in classes the tests register163 tests pass on Python 3.11 and 3.13, and pre-commit, including strict mypy, passes.