-
Notifications
You must be signed in to change notification settings - Fork 511
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
[Core] Task level config #3689
[Core] Task level config #3689
Conversation
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.
Thanks for adding this feature @Michaelvll ! Left some comments to discuss :)
resources_config = config.pop('resources', {}) | ||
if cluster_config_override is not None: | ||
assert resources_config.get('_cluster_config_overrides') is None, ( | ||
'Cannot set _cluster_config_overrides in both resources and ' |
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.
Should we raise an error if we find user specified resources._cluster_config_overrides
as this is not allowed (according to the comment in the schema)?
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.
We currently rely on the same API to check the YAML provided by the user and the YAML generated by SkyPilot internally. It may require some refactoring, and it may not be very urgent to do such check as we do not expose this field in our docs. : )
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.
Good point! Lets keep the current one
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.
Thanks for the fix @Michaelvll ! LGTM.
* Add docker run options * Add docs * Add warning for docker run options in kubernetes * Add experimental config * fix * rename vars * type * format * wip * rename and add tests * Fixes and add tests * format * Assert for override configs specification * format * Add comments * fix * fix assertions * fix assertions * Fix test * fix * remove unsupported keys * format
Allow task-level configuration.
Blocked by #3682Tested (run the relevant ones):
bash format.sh
pytest tests/test_config.py
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
conda deactivate; bash -i tests/backward_compatibility_tests.sh