[SkyServe] Support mixture of spot and on-demand #3194
Conversation
|
@cblmemo Thanks! resolved all the comments. |
| # blocking probe to other replicas. | ||
| is_preempted = self._handle_preemption(info) | ||
| if is_preempted: | ||
| continue |
There was a problem hiding this comment.
The whole for loop of the future in probe_futures is quite weird, why can't we just have the whole "probe and process probe results" in a same function and have that function to be parallelized, instead of only parallelizing info.probe. cc'ing @cblmemo
Otherwise the processing and termination of the unhealthy replica could block the whole process of the probing.
There was a problem hiding this comment.
Agreed. Actually this is taking a lot of time and once caused some bugs in our SpotHedge e2e experiment; we should parallelize the preemption handling as well
There was a problem hiding this comment.
should we spawn another thread to handle preemption (for modularity)?
There was a problem hiding this comment.
I'm feeling like we could spawn a thread to do all of the following:
info.probe- Update all attributes in
info, e.g.info.status_property - Handle preemption
- Update database
Does that sounds good to you?
There was a problem hiding this comment.
I am not sure if we should mix all these tasks under one thread. This probably requires more discussion and I think can be a different PR.
tests/test_smoke.py
Outdated
| def _check_both_spot_on_demand_in_status(name: str) -> str: | ||
| return (f'output=$(sky serve status {name});' | ||
| 'echo "$output" | grep "GCP(\[Spot\]vCPU=2)" && ' | ||
| 'echo "$output" | grep "GCP(vCPU=2)";') |
There was a problem hiding this comment.
Actually that makes me think, should we print the is_spot column in our sky serve status table?
There was a problem hiding this comment.
The row will have \[Spot\] so I think we don't need a is_spot column
There was a problem hiding this comment.
Thanks for fixing this PR @MaoZiming! The code looks mostly good to me.
…o serve-spot-no-placer
#2793
without spot placer.
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