Allow empty YAML fields

[iojw]

Users may often comment out the nested contents of YAML fields without remove the top-level field. Previously, this would throw the following error due to our YAML validation:

ValueError: Invalid task YAML: None is not of type 'string'. Check problematic field(s): $.run

This PR updates it so that such fields are treated as though they do not exist. It does this by removing all fields with a None value in the parsed dictionary before validation.

We do not solve this issue by adding null to the JSON schema for our YAML config because many places in the code test for the existence of specific fields in the dictionary so we would have to modify all of their logic. Moreover, having to add null to every single field in the JSON schema reduces readability.

Tested (run the relevant ones):

• Tested with YAML configs with empty fields
• Added unit test for yaml parsing
• 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

[concretevitamin]

Thanks @iojw. A nit. Consider adding a unit test to ensure this doesn’t break in the future?

[concretevitamin]

Possible to move this into backend_utils.validate_schema since there are three places?

[iojw]

I've added a flag so we can enable this for validation. The reason we don't want to do it for all cases is that it prevents us from actually having null values in the schema in the future.

In addition, a lot of the current code relies on the fact that if a key exists in the config dictionary, then the value is valid. E.g. if we allow None to be inside the config dictionary, then inside storage.from_yaml_config, force_delete = config.pop('_force_delete', False) would incorrectly set force_delete to None if the user specifies force_delete in the YAML without a value. Therefore, I also had to refactor the code such that it properly handle None config values.

[concretevitamin]

Have we ensured that for all fields in the current YAML spec, None means absence means default value? Any cases where None means something else?

[iojw]

@concretevitamin Yes, I've looked through Resources, Storage, and Task to ensure that for these schemas, None does not have a special meaning and is properly handled.

[concretevitamin]

Thanks, some questions.

[concretevitamin]

When do we want skip_none=False? In cli.py’s admin_deploy()?

[iojw]

We may want it in cases where None is a valid value for some config we are validating. After thinking about it further, it makes more sense to set the default to True, since it is likelier than we want to remove None values than not. I've updated to code for this.

[concretevitamin]

@iojw Oops, slipped through the radar. Could you merge with master first?

[iojw]

@concretevitamin Updated!

[concretevitamin]

LGTM. With the latest commit,

FAILED tests/test_smoke.py::test_gcp_stale_job_manual_restart - AttributeError: module 'sky.skylet.events' has no attribute 'JobUpdateEvent'. Did you mean: 'SpotJobUpdateEvent'?
FAILED tests/test_smoke.py::test_file_mounts - Exception: test failed: less /tmp/using_file_mounts-bsvqi8s_.log
FAILED tests/test_smoke.py::test_large_job_queue - Exception: test failed: less /tmp/large_job_queue-x96b5ene.log
FAILED tests/test_smoke.py::test_job_queue_multinode - Exception: test failed: less /tmp/job_queue_multinode-iyo5bv9n.log
FAILED tests/test_smoke.py::test_multi_echo - Exception: test failed: less /tmp/multi_echo-86fyim3q.log
FAILED tests/test_smoke.py::test_env_check - Exception: test failed: less /tmp/env_check-v7b2xnyx.log
6 failed, 62 passed, 35 skipped, 1646 warnings in 3462.05s (0:57:42)

These tests failed, most likely due to merge conflicts. See comment: https://github.com/skypilot-org/skypilot/pull/1636/files#r1220710715

Could you merge again?

[concretevitamin]

LGTM, thanks @iojw.

[iojw]

Confirmed that smoke tests pass with these changes.