Skip to content
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

Implement config validation #2645

Merged
merged 17 commits into from
Oct 24, 2023
Merged

Implement config validation #2645

merged 17 commits into from
Oct 24, 2023

Conversation

iojw
Copy link
Collaborator

@iojw iojw commented Oct 3, 2023

Closes #2474

Implements validation for the ~/.sky/config.yaml file. If there is an error, an exception is thrown and the user is warned.

To avoid circular imports, I had to refactor the code a bit, moving validation code into common_utils and moving the imports in sky.util.schemas to within the functions.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Pass existing config tests, added new unit tests
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting the PR @iojw! The code looks mostly good to me, but there seems to be a circular import, when I tried sky check with the current PR.

sky/data/storage.py", line 49, in <module>
    str(clouds.AWS()),
AttributeError: partially initialized module 'sky.clouds' has no attribute 'AWS' (most likely due to a circular import)

sky/skypilot_config.py Outdated Show resolved Hide resolved
@iojw
Copy link
Collaborator Author

iojw commented Oct 3, 2023

Updated to throw an exception and also to fix the circular import issue mentioned (it's caused by the kubernetes_utils import in schemas.py so I've moved the enums to a separate module).

Strangely, I'm unable to reproduce it locally, so could you help to verify that it is fixed @Michaelvll ?

@iojw iojw requested a review from Michaelvll October 4, 2023 19:56
Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the issues and fixing them @iojw! It looks mostly good. Left some minor comments. cc'ing @concretevitamin to see if he has some additional test to be run. : )

sky/skypilot_config.py Outdated Show resolved Hide resolved
sky/utils/kubernetes_enums.py Show resolved Hide resolved
sky/utils/schemas.py Show resolved Hide resolved
sky/utils/schemas.py Show resolved Hide resolved
sky/utils/schemas.py Outdated Show resolved Hide resolved
sky/utils/schemas.py Outdated Show resolved Hide resolved
@iojw iojw requested a review from Michaelvll October 12, 2023 22:43
Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the schema check @iojw! LGTM with an additional comment. Would be nice to do some more manual test to avoid any accident false positive schema alert.

docs/source/reference/config.rst Outdated Show resolved Hide resolved
sky/utils/schemas.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @iojw! Left some comments on the k8s config docs, otherwise LGTM!

docs/source/reference/config.rst Outdated Show resolved Hide resolved
docs/source/reference/config.rst Outdated Show resolved Hide resolved
docs/source/reference/config.rst Outdated Show resolved Hide resolved
docs/source/reference/config.rst Outdated Show resolved Hide resolved
docs/source/reference/config.rst Outdated Show resolved Hide resolved
@iojw
Copy link
Collaborator Author

iojw commented Oct 24, 2023

Thank you @romilbhardwaj for taking a look! I'll merge this in soon if there are no further comments.

@iojw iojw merged commit dd9450d into master Oct 24, 2023
18 checks passed
@iojw iojw deleted the config-validation branch October 24, 2023 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[config] Pre-checks for ~/.sky/config.yaml
4 participants