Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Paragon SummaryThis pull request review analyzed 8 files and found no issues. The review examined code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. Paragon did not detect any problems in the current diff. Proceed with merge after your normal checks. This PR fixes how the Trackio working directory is determined and propagated when launching jobs and sweeps, so runs use the correct path consistently across the backend, SDK, and UI. It also updates related configuration to keep the compute-provider flow aligned with that corrected behavior. Key changes:
Confidence score: 5/5
8 files reviewed, 0 comments Tip: |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
dadmobile
left a comment
dadmobile
left a comment
There was a problem hiding this comment.
Leaving some factoring recommendations but you might judge some to be more work than worth. But have a look and make a call?
| env_vars["_TFL_EXPERIMENT_ID"] = request.experiment_id | ||
| env_vars["_TFL_USER_ID"] = user_id | ||
|
|
||
| trackio_project_name_for_child: str | None = None |
There was a problem hiding this comment.
Wondering if there's an opportunity to factor out some sort of shared helper here? Seems like we ahve 3 separate places where we figure out a project name, set some VARS, build up a path, write a thing.
I know each case is slightly different so possibly I'm over simplifying?
There was a problem hiding this comment.
Okay I'll put the trackio dir logic in lab.dirs but for the other things like run name and project, it all depends on each type of launch right instead of common logic?
There was a problem hiding this comment.
Okay I was able to build a shared helper for all of the common trackio logic and I also did the lab.dirs thing so it should cover everything
| temp_dir = f"/tmp/trackio/{job_id_env}" | ||
| os.makedirs(temp_dir, exist_ok=True) | ||
| os.environ["TRACKIO_DIR"] = temp_dir | ||
| trackio_dir = (os.environ.get("TRACKIO_DIR") or "").strip() |
There was a problem hiding this comment.
Maybe another place for factoring...same as line 176 below?
There was a problem hiding this comment.
Fixed to get through get_trackio_dir
| trackio_run_name_for_child = st_run_name | ||
| env_vars["TLAB_TRACKIO_PROJECT_NAME"] = st_project_name | ||
| env_vars["TLAB_TRACKIO_RUN_NAME"] = st_run_name | ||
| env_vars["TRACKIO_DIR"] = f"/tmp/trackio/{child_job_id}" |
There was a problem hiding this comment.
Hardcode /tmp/trackio in a bunch of places. I wonder if we put in DIRS or something?
2 participants
No description provided.