-
-
Notifications
You must be signed in to change notification settings - Fork 540
Add ability to mark jobs as discarded and also add scoring #1965
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
fc722a7
b66dcdd
590c74e
f88ba05
dc97c09
e6d230e
c836b44
9d459ec
06bfa26
607fb77
17eb833
8b4d591
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,10 +131,21 @@ async def job_delete(job_id: str, experimentId: str): | |
|
|
||
| @router.put("/{job_id}/job_data") | ||
| async def job_update_job_data(job_id: str, experimentId: str, body: dict = Body(...)): | ||
| """Update user-facing metadata fields in job_data (favorite, hidden, tags).""" | ||
| """Update user-facing metadata fields in job_data (favorite, hidden, tags, discard).""" | ||
| updates = body.get("updates", {}) | ||
| ALLOWED_KEYS = {"favorite", "hidden", "tags"} | ||
| filtered = {k: v for k, v in updates.items() if k in ALLOWED_KEYS} | ||
| allowed_keys = {"favorite", "hidden", "tags", "discard"} | ||
| filtered = {k: v for k, v in updates.items() if k in allowed_keys} | ||
|
|
||
| # Keep discard under job_data.score.discard to avoid introducing a new top-level field. | ||
| if "discard" in filtered: | ||
| discard_value = bool(filtered.pop("discard")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably fine but as a note "false" will evaluate as true I think.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay fixed this and also found similar things in CLI and frontend, fixed both
dadmobile marked this conversation as resolved.
Outdated
dadmobile marked this conversation as resolved.
Outdated
deep1401 marked this conversation as resolved.
Outdated
|
||
| existing_job = await job_service.job_get_cached(job_id, experiment_id=experimentId) | ||
| existing_job_data = (existing_job or {}).get("job_data", {}) if isinstance(existing_job, dict) else {} | ||
| existing_score = existing_job_data.get("score", {}) if isinstance(existing_job_data, dict) else {} | ||
| merged_score = dict(existing_score) if isinstance(existing_score, dict) else {} | ||
| merged_score["discard"] = discard_value | ||
| filtered["score"] = merged_score | ||
|
|
||
| if not filtered: | ||
| return {"message": "No valid keys to update"} | ||
| await job_service.job_update_job_data_insert_key_values(job_id, filtered, experimentId) | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -638,8 +638,11 @@ def finish( | |
| except Exception: | ||
| # Never let optional Trackio integration break finish() | ||
| logger.debug("Trackio integration failed during finish()", exc_info=True) | ||
| if score is not None: | ||
| _run_async(self._job.update_job_data_field("score", score)) # type: ignore[union-attr] | ||
| resolved_score: Dict[str, Any] = {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is OK but just double checking: We don't set score anywhere earlier? I just noticed that if you call finish without a score I think it will overwrite if there's anythign in there already. But I think that's fine?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No score is only set by finish, nowhere before that |
||
| if isinstance(score, dict): | ||
| resolved_score.update(score) | ||
| resolved_score.setdefault("discard", False) | ||
| _run_async(self._job.update_job_data_field("score", resolved_score)) # type: ignore[union-attr] | ||
| if additional_output_path is not None and additional_output_path.strip() != "": | ||
| _run_async(self._job.update_job_data_field("additional_output_path", additional_output_path)) # type: ignore[union-attr] | ||
| if plot_data_path is not None and plot_data_path.strip() != "": | ||
|
|
||
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.
Technically this is race. Not sure how important (not super worried about two people flipping discard on same job at the same time, but maybe UI and SDK fighting is possible).
OK I asked Claude for a simple fix and it said to create a function at service later and call from here if "discard" is passed. Something like this (note this is technically still a race, just narrower and cleaner):
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.
Fixed now based on what you recommended