[Core] Support inherit base resources with any_of and ordered in resources field

[Michaelvll]

Fixes #2831

Tested (run the relevant ones):

• Code formatting: bash format.sh
• Any manual or new tests for this PR (please specify below)
  • sky launch -c test-resources echo hi
  • sky launch -c test-resources --cpus 2 echo hi
  • sky spot launch -n test-resources --cpus 2 echo hi with ~/.sky/config.yaml specifying controller's cloud.
  • sky spot launch -n test-resources --cpus 2 "echo hi; sleep 10000" manually terminate and recover.
  • sky serve up examples/serve/http_server/task.yaml
• All smoke tests: pytest tests/test_smoke.py
• Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • pytest tests/test_smoke.py::test_multiple_a ccelerators_ordered_with_default
  • pytest tests/test_smoke.py::test_multiple_accelerators_unordered_with_default
• Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

[MaoZiming]

why do we limit controlller_resources to 1?

[Michaelvll]

It seems that SkyServe requires a single candidate of controller resources, e.g.:

skypilot/sky/serve/core.py

Lines 105 to 115 in 99f04b1

	controller_cloud = (
	requested_resources.cloud if not controller_exist and
	controller_resources.cloud is None else controller_resources.cloud)
	# TODO(tian): Probably run another sky.launch after we get the load
	# balancer port from the controller? So we don't need to open so many
	# ports here. Or, we should have a nginx traffic control to refuse
	# any connection to the unregistered ports.
	controller_resources = controller_resources.copy(
	cloud=controller_cloud,
	ports=[serve_constants.LOAD_BALANCER_PORT_RANGE])
	controller_task.set_resources(controller_resources)

I think it might be fine to have a more strict constraint for the controller resources at the moment.

[MaoZiming]

sounds good.

[MaoZiming]

I am wondering if the fields under any_of or ordered conflict with what is above any_of or ordered.
Will the field under any_of overwrite the fields above any_of?

[Michaelvll]

I am wondering if the fields under any_of or ordered conflict with what is above any_of or ordered. Will the field under any_of overwrite the fields above any_of?

Yes, my current thinking is to have the field under any_of or ordered to overwrite the configs outside any_of/ordered.

[MaoZiming]

@Michaelvll Okay make sense.

[MaoZiming]

lgtm after fixing pylint

[cblmemo]

Thanks for adding this feature!! Sorry for missed it. Left several things to discuss 🫡

[cblmemo]

Just did another pass and left several nits & one discussion. If we decided to keep the accelerators: {L4:1, T4:1} as a shortcut/backward compatibility/any other reason, then this version of the code looks great to me!

[cblmemo]

Thanks for the fix! I tried the following in latest commit:

>>> import sky
>>> sky.Resources.from_yaml_config({'use_spot': True, 'accelerators': {'A100-40GB:1': None, 'T4:1': None, 'V100:1': None}, 'any_of': [{'cloud': 'aws'}, {'cloud': 'gcp'}]})
ValueError: Invalid resources YAML: {'A100-40GB:1': None, 'T4:1': None, 'V100:1': None} is not valid under any of the given schemas. Check problematic field(s): $.accelerators
>>> sky.Resources.from_yaml_config({'use_spot': True, 'accelerators': 'A100-40GB:1', 'any_of': [{'cloud': 'aws'}, {'cloud': 'gcp'}]})
AssertionError: Invalid resource args: dict_keys(['any_of'])

Is this expected? Correct me if I'm wrong, but I suppose we should:

1. allow set of accs;
2. allow any_of.
   in Python API too?

[cblmemo]

I'm slightly confused about why we could still use the old schema check. Should we add the any_of and ordered fields in the resources schema?

[Michaelvll]

We already have the any_of and ordered field included in the schema in #2498

[Michaelvll]

sky.Resources.from_yaml_config({'use_spot': True, 'accelerators': {'A100-40GB:1': None, 'T4:1': None, 'V100:1': None}, 'any_of': [{'cloud': 'aws'}, {'cloud': 'gcp'}]})

Could you double check if you are using the correct commit? It seems working correctly for me with the current branch.

[cblmemo]

Thanks for adding this! Pls ignore the error above; seems like I used the wrong branch. LGTM!

[cblmemo]

Suggested change

	raise RuntimeError('Accelerators must be a list or a set.')
	raise RuntimeError('Accelerators must be a list or a set when multiple accelerators are specified.')

[Michaelvll]

Good call! Actually, I find this should be assertion instead of raise, as the type of the resources should have ben guaranteed by the schema check.