-
Notifications
You must be signed in to change notification settings - Fork 654
refactor(config): implement three-phase Parse → Validate → Complete architecture #2696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
73957d3 to
7260c9b
Compare
…rchitecture This refactors the config system to use a cleaner three-phase architecture: 1. **Parse**: YAML → ConfigFile (raw representation with pointer fields) 2. **Validate**: Pure validation with no mutations, returns structured errors 3. **Complete**: ConfigFile → Config with defaults and CUDA resolution - errors.go: Structured error types (ParseError, SchemaError, ValidationError, etc.) - config_file.go: Raw ConfigFile type mirroring YAML with pointer fields - parse.go: Parse(), ParseBytes(), ParseReader(), FromYAML() functions - validate.go: ValidateConfigFile() with functional options pattern - complete.go: CompleteConfig() for transforming ConfigFile → Config - schema.go: GenerateSchema() using github.com/invopop/jsonschema - build_options.go: BuildOptions struct for CLI flags (alternative to globals) - validator.go: Old schema validation (replaced by validate.go) - validator_test.go: Old tests (replaced by validate_test.go) - Load() returns LoadResult with Config, Warnings, and RootDir - Complete() replaces ValidateAndComplete() for programmatic configs - FromYAML() is now a test helper that doesn't run completion - Removed GetConfig(), GetRawConfig(), loadConfigFromFile() - Renamed JSON files for consistency (*_matrix.json → *_compatibility.json) - Renamed env_variables.go → env.go - Updated callers to use new APIs
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
Bug fixes: - Fix major version check in validateConcurrency using splitPythonVersion - Use filepath.Join instead of path.Join for OS-specific path handling - Remove dead code (empty if !found block) in validateFrameworkCompatibility - Remove unused schemaVersion constant Dead code removal: - Remove unused Version, Image, Stats structs from version.go - Remove unused Example struct from config.go Unexport internal code: - ValidateCudaVersion → validateCudaVersion - CUDABaseImageFor → cudaBaseImageFor - ConfigFile/BuildFile/RunItemFile/etc → lowercase versions - EnvironmentVariableDenyList → environmentVariableDenyList
- Use filepath.Join instead of path.Join in load.go for OS-specific paths - Fix ValidateModelPythonVersion to check major version for concurrency - Remove unused getter methods from config_file.go (GetPythonVersion, GetPythonRequirements, GetCUDA, GetCuDNN, GetMax) - Standardize error messages to lowercase per Go conventions - Use %w for error wrapping in findConfigPathInDirectory
29481c8 to
60dcbf2
Compare
- Fix golangci-lint issues: rename shadowed 'max' variable, use type conversion - Remove duplicate ValidateModelPythonVersion function from config.go - Remove redundant validation call from build.go (Load already validates) - Update integration tests to match new ValidationError message format
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
tempusfrangit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pretty reasonable. Please make sure wip is removed from the PR title. I'd also like @michaeldwan 's review on this before we merge.
| if len(c.Build.PythonPackages) > 0 { | ||
| c.Build.pythonRequirementsContent = reqs | ||
| } else if len(c.Build.PythonPackages) > 0 { | ||
| // Backwards compatibility: if using deprecated python_packages, populate requirements content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's time to drop this and error. we're doing a TON of big restructuring and this has been deprecated for a while. We can do this as a separate PR before release. I expect 0.17.0 can include this PR and the future removal of deprecated things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this sentiment. Let's delete more things!
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
Signed-off-by: Mark Phelps <mphelps@cloudflare.com>
Config Refactor
1. Clear Three-Phase Architecture (Parse → Validate → Complete)
Before (main): Config loading was a monolithic process where parsing, validation, and CUDA resolution were interleaved and hard to follow.
After: Clean separation of concerns:
parse.go) - Only YAML parsing, no side effectsvalidate.go) - Semantic validation with typed errorsconfig.go) - CUDA resolution, requirements loadingThis makes the code easier to understand, test, and maintain.
2. Typed Error System
Before: Generic
errorreturns with string formatting, hard to programmatically handle different error types.After: Rich error types in
errors.go:ParseError- YAML parsing failuresSchemaError- Schema validation issuesValidationError- Semantic validation failuresDeprecationWarning- Deprecated field usageCompatibilityError- Version incompatibilitiesCallers can use
errors.As()to handle specific error types differently.3. Separation of Raw Config from Completed Config
Before: Single
Configstruct used for both raw YAML and completed config, with nullable fields mixed with populated fields.After:
configFile(unexported) - Raw YAML with pointer fields to distinguish "not set" from "zero value"Config- Completed config with resolved values4. Better Deprecation Handling
Before: Deprecation warnings were ad-hoc and mixed with errors.
After:
ValidationResultseparates errors from warnings:Can optionally treat deprecations as errors with
WithStrictDeprecations().5. Functional Options Pattern for Validation
Before: Hard-coded validation behavior.
After: Flexible configuration:
6. Bug Fixes
filepath.Joinvspath.Join)7. Reduced API Surface
configFile,buildFile, etc.)validateCudaVersion,cudaBaseImageFor)Example,Version,Statsstructs)8. Better Testability
WithRequirementsFS()allows testing without real filesystem9. Idiomatic Go
%wslicespackage with stdlib