- Notifications
You must be signed in to change notification settings - Fork6.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Chore][Core/Dashboard] Remove TrainHead's dependency on DataOrganizer#51739
[Chore][Core/Dashboard] Remove TrainHead's dependency on DataOrganizer#51739
Conversation
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
… infosSigned-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
| actor_ids_qs_str = ",".join(actor_ids) | ||
| url = f"http://{self.http_host}:{self.http_port}/logical/actors?ids={actor_ids_qs_str}&nocache=1" | ||
| async with self._http_session.get(url) as resp: | ||
| assert resp.status == 200, f"Failed to get actor info: {resp.status}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lets' not use assert, but returns the 500 to the end user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It already returns 500 because this will raiseAssertionError. This function is called by other http route handlers. And if this error is not caught, the route handler will return 500 internal server error response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
assert is used to catch program logical errors. Lets raise a different error instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated. Useresp.raise_for_status() instead.
| actor_infos = await DataOrganizer.get_actor_infos( | ||
| actor_ids=[train_run.controller_actor_id], | ||
| ) | ||
| actor_infos = await self._get_actor_infos([train_run.controller_actor_id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is the format the same between what returns from http and what returns from DataOrganizer directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes. They are the same.
Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
ray-project#51739)Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>Signed-off-by: Justin Miller <justinrmiller@gmail.com>
ray-project#51739)Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Why are these changes needed?
This PR removes
TrainHead's dependency onDataOrganizer.nocachequery string support to theaiohttp_cachedecorator. If it's set to"1", the cache will be bypassed.test_aiohttp_cachetest:Counterwithset, which makes more sense here.whileloop in the test since we don't need to retry for this.nocachequery string.idsquery string support toActorHead. We can now useGET /logical/actors?ids=id1,id2,id3to fetch info for multiple actors. Also added a test for this.TrainHeadto stop callingDataOrganizer.get_actor_infosdirectly. It now makes an HTTP request toActorHead, which will callDataOrganizer.get_actor_infoson its behalf.Related issue number
N/A
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.