feat(FR-2592): validate deployment-config.yaml in the vfolder file editor - #9514
feat(FR-2592): validate deployment-config.yaml in the vfolder file editor#9514yomybaby wants to merge 2 commits into
Conversation
…itor The model-folder deployment config moved from `service-definition.toml` to `deployment-config.yaml`: the manager reads the yaml first and keeps the toml only as a deprecated fallback (manager repositories/deployment/repository.py:122-123, :572-573), and the shipped manual documents it that way. The Monaco schema map in the vfolder file editor still knew only the legacy filename, so the current format got no validation or completion at all. - Add `resources/deployment-config.schema.json`, authored from the manager's `DeploymentConfigInput` (repositories/deployment/storage_source/storage_source.py:26-40) and the manual's field reference (packages/backend.ai-webui-docs/src/en/deployment/deployment.md:414-470): the root-level `environment` / `image` / `architecture` / `resource_slots` / `resource_opts` / `environ` defaults, plus runtime-variant sections as additional top-level keys. - Map `deployment-config.yaml` to it. No `.yml` twin — the manager's candidate list names only `deployment-config.yaml`. - Repoint the legacy `service-definition.toml` entry at the same schema. The manager parses both filenames into the same payload, while `service-definition.schema.json` describes the unrelated container service-def files under /etc/backend.ai/service-defs/ and required a `command` key a model-folder file never has, so every valid file was warned about. The entry stays, marked deprecated. Every field is optional and validation is warning-level, so no existing file starts failing. Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
Coverage Report for react-coverage (./react)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Changes recommended
The schema currently validates unsupported configuration forms and mishandles empty YAML documents.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Monaco validation and completion for deployment configuration files.
Changes:
- Adds a deployment configuration JSON schema.
- Maps current YAML and legacy TOML filenames to the schema.
- Adds schema validation tests.
Unresolved issues:
- Runtime-variant sections and nested
environmentvalues are accepted but ignored by the manager. - Empty YAML parses as
null, which the schema rejects. - The empty-file test validates
{}instead of exercisingvalidateYaml('').
File summaries
| File | Description |
|---|---|
resources/deployment-config.schema.json |
Defines deployment configuration validation; requires alignment with manager behavior and empty YAML handling. |
react/src/helper/deploymentConfigSchema.test.ts |
Tests schema behavior but does not cover the actual empty-file parsing path. |
react/src/components/VFolderTextFileEditorModal.tsx |
Registers schema mappings for both filenames. |
Review details
Suppressed comments (1)
react/src/helper/deploymentConfigSchema.test.ts:64
- This bypasses the YAML parsing path and validates
{}, so it does not cover an actually empty file:parse('')yieldsnull, which the current schema rejects. ExercisevalidateYaml('')here so the production behavior is protected.
it('accepts an empty file — every field is optional', () => {
expect(validate({})).toBe(true);
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| "environment": { | ||
| "$ref": "#/$defs/Environment" | ||
| }, |
| "additionalProperties": { | ||
| "$ref": "#/$defs/RuntimeVariantSection" |
The editor validates `validate(parse(text))`, and `yaml.parse('')` returns
null, so a root `"type": "object"` made every brand-new (or comment-only)
deployment-config.yaml warn `root: must be object` — exactly the file the
PR's review notes ask a reviewer to create. The root type now also accepts
null.
The empty-file test asserted `validate({})`, which bypasses the parse path
and so could not catch this; it now goes through `validateYaml('')` and a
comment-only document, with the empty-mapping case kept separately.
Claude-Session: https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ
There was a problem hiding this comment.
🔵 Needs a closer look
The schema exposes unsupported Manager fields, and dynamic runtime sections lack completion support.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
resources/deployment-config.schema.json:37
- Runtime-variant fields validate here, but they receive no completion.
getSchemaAtPath()inmonacoYamlCompletion.tsonly follows namedproperties; for a path such asvllm, it returnsundefinedinstead of following this schema-valuedadditionalProperties, so typing insidevllm:yields no suggestions. Add anadditionalPropertiesfallback to the completion traversal and cover a dynamic section in its tests.
resources/deployment-config.schema.json:9
- The current Manager contract does not consume this nested object (or the arbitrary runtime-variant sections below):
DeploymentConfigInputdeclares only flatimage,architecture,resource_slots,resource_opts, andenvironfields withextra="ignore", and the repository reads those flat attributes directly. Consequently, the editor now validates and suggests configurations whose image/variant defaults are silently discarded at deployment time. Either add Manager-side normalization/support first, or restrict this schema (and the manual) to the fields the Manager actually consumes.
"environment": {
"$ref": "#/$defs/Environment"
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Resolves #6750 (FR-2592)
What changed
The model-folder deployment config moved from
service-definition.tomltodeployment-config.yaml. The manager reads the yaml first and keeps the toml only as a deprecated fallback (repositories/deployment/repository.py:122-123,:572-573), and the shipped manual documents exactly that relationship (packages/backend.ai-webui-docs/src/en/deployment/deployment.md:414-470). The Monaco schema map in the vfolder file editor still knew only the legacy filename, so the current format got no validation and no completion at all.resources/deployment-config.schema.json. Authored from the manager'sDeploymentConfigInput(src/ai/backend/manager/repositories/deployment/storage_source/storage_source.py:26-40—image,architecture,resource_slots,resource_opts,environ,extra="ignore") and the manual's field reference. Covers the root-level defaults, the documentedenvironment:grouping, and runtime-variant sections (any other top-level key, per the manual's three-level override hierarchy).deployment-config.yaml→ that schema indefinitionSchemaMap. The yaml validator + completion wiring already exists (it servesmodel-definition.yaml), so no new plumbing.service-definition.tomlentry now points at the same schema and is marked deprecated. It stays mapped, since the manager still reads it.Design decisions
deployment-config.ymltwin. The manager's candidate list names onlydeployment-config.yaml(repository.py:122), unlikemodel-definition, whose path the caller supplies. Mapping.ymlwould give editor help for a filename the manager never reads.resources/service-definition.schema.jsondescribes a different artifact — the container service-def JSON files under/etc/backend.ai/service-defs/($id/description at :3-5;required: ["command"]). A model-folderservice-definition.tomlhas none of those keys, so main today warns on every valid file. The manager parses both filenames into the sameDeploymentConfigInput, so sharing the schema is what matches reality.service-definition.schema.jsonis left in place — it still describes the container files.environment: {image, architecture}and root-levelimage/architecture. The manual documents the nested grouping; the manager reads the flat pair. Declaring only one would make the other warn.requiredand validation is warning-level (monacoSchemaValidator.tsemitsMarkerSeverity.Warning), so no existing file starts failing.Out of scope, left alone: the two stale doc comments in
VFolderNodes*.tsxand the e2e fixtures that still use the legacy filename — the legacy name is still supported, so those assertions remain valid. No i18n key references either filename any more.Tests
react/src/helper/deploymentConfigSchema.test.ts— compiles the schema with the sameAjv2020({ allErrors: true })the editor uses and asserts: the manual's full documented example validates, the manager's flat root pair validates, an empty file validates, and a non-stringenvironvalue / a scalar runtime-variant section / a non-scalar resource slot each produce a finding at the right instance path.pnpm exec vitest run src/helper/deploymentConfigSchema.test.ts→ 6 passed.bash scripts/verify.sh(full harness).Verification
Review notes
deployment-config.yaml, and edit it: typing at the root should completeenvironment/resource_slots/resource_opts/environ, andenviron: { PORT: 8080 }should raise a warning squiggle (values must be strings).service-definition.tomlin a model folder: the spuriousroot: must have required property 'command'warning from the container service-def schema is gone.resources/tree (copyresourceinpackage.json:34), same as the two existing*.schema.jsonfiles — nothing else to wire up.Checklist: (if applicable)
deployment-config.yamlas current and the toml as a deprecated fallback.https://claude.ai/code/session_011RFmSEBxxxvCXyquSPtqVJ