[Tests] Fix optimizer test by leaving out unsupported clouds #2976
Conversation
There was a problem hiding this comment.
Thanks @Michaelvll! LGTM. Left one minor comment and a question.
tests/test_optimizer_random_dag.py
Outdated
| resources.cloud.check_features_are_supported( | ||
| resources, requested_features) | ||
| except exceptions.NotSupportedError: | ||
| continue |
There was a problem hiding this comment.
Quick question - instead of continuing, can we set op.num_nodes = 1 and then pass? That way we can still include clouds which don't support multi-node in this test. (I may be missing something here)
except exceptions.NotSupportedError:
op.num_nodes = 1
There was a problem hiding this comment.
We randomly set the op.num_nodes at the beginning so it should have some task that has single node, but this is a good point. I now enforce some of the tasks to be single node.
There was a problem hiding this comment.
Seems like we could end up having no candidate resources if it chooses a cloud that does not support multiple-node too many times? Maybe change to
while True:
candidate = random.choice(ALL_INSTANCE_TYPE_INFOS)instead of using a fixed-sized array?
There was a problem hiding this comment.
It seems we are already choosing from the whole list of ALL_INSTANCE_TYPE_INFOS by setting k=len(ALL_INSTANCE_TYPE_INFOS). It should be fine with the current way? Using while True may cause unexpected infinite loop if all instance type fails?
tests/test_optimizer_random_dag.py
Outdated
| resources.cloud.check_features_are_supported( | ||
| resources, requested_features) | ||
| except exceptions.NotSupportedError: | ||
| continue |
There was a problem hiding this comment.
Seems like we could end up having no candidate resources if it chooses a cloud that does not support multiple-node too many times? Maybe change to
while True:
candidate = random.choice(ALL_INSTANCE_TYPE_INFOS)instead of using a fixed-sized array?
Co-authored-by: Tian Xia <cblmemo@gmail.com>
Co-authored-by: Tian Xia <cblmemo@gmail.com>
… into test-optimizer-fix
Separating this fix from #2829, as it may affect other clouds as well.
Our brute force includes the clouds that do not support multiple nodes, while the optimizer does not, which will cause issues.
This is blocking #2951
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_namebash tests/backward_comaptibility_tests.sh