Skip to content

Move jobs within the experiment folder and make calls faster#1607

Merged
deep1401 merged 32 commits intomainfrom
fix/jobs-list-fetcher
Mar 25, 2026
Merged

Move jobs within the experiment folder and make calls faster#1607
deep1401 merged 32 commits intomainfrom
fix/jobs-list-fetcher

Conversation

@deep1401
Copy link
Copy Markdown
Member

@deep1401 deep1401 commented Mar 23, 2026 •
edited
Loading

No description provided.

@sentry
Copy link
Copy Markdown

sentry bot commented Mar 23, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 34.93333% with 244 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...merlab/services/migrate_jobs_to_experiment_dirs.py 32.07% 97 Missing and 11 partials ⚠️
api/transformerlab/routers/experiment/jobs.py 34.73% 62 Missing ⚠️
api/transformerlab/services/job_service.py 47.72% 39 Missing and 7 partials ⚠️
api/transformerlab/routers/compute_provider.py 6.66% 14 Missing ⚠️
api/transformerlab/services/trackio_service.py 14.28% 6 Missing ⚠️
api/transformerlab/shared/shared.py 20.00% 4 Missing ⚠️
api/transformerlab/services/quota_service.py 0.00% 3 Missing ⚠️
...pi/transformerlab/services/sweep_status_service.py 50.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@deep1401 deep1401 changed the title Fix/jobs list fetcher Move jobs within the experiment folder and make calls faster Mar 23, 2026
@deep1401 deep1401 marked this pull request as ready for review March 23, 2026 16:36
dadmobile
dadmobile requested changes Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Havne't tested yet. But found some things with job_get that might cause issues?

dadmobile
dadmobile requested changes Mar 24, 2026
View reviewed changes
api/transformerlab/services/job_service.py
if not resolved_job_id:
return None

key = _job_cache_key(resolved_job_id)
Copy link
Copy Markdown
Member

@dadmobile dadmobile Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just calling this out as I'm not 100% I'm following...
In the get function this makes sure to resolve the job_id to to the full value but in job_update_status it uses the passed job_id, which I'm not sure if it will definitely be the resolved full ID or not?

Copy link
Copy Markdown
Member Author

@deep1401 deep1401 Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wont get to that case because I think we use full ids everywhere for cache writes and the read comes through Job.get which would need the full id so there wont be duplicated cache entries?

api/transformerlab/services/migrate_jobs_to_experiment_dirs.py Outdated
Disable auto-migration when `TFL_DISABLE_MIGRATE_JOBS=1` (or true-ish).

Also supports a legacy/alternative value:
- `TFL_DISABLE_MIGRATE=jobs`
Copy link
Copy Markdown
Member

@dadmobile dadmobile Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this exist before?!?

Copy link
Copy Markdown
Member Author

@deep1401 deep1401 Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didnt exist before but I just added it as a dev thing incase someone wanted to use. Should I get rid of it?

Copy link
Copy Markdown
Member

@dadmobile dadmobile Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I meant that for some reason there are two variables? TFL_DISABLE_MIGRATE_JOB and TFL_DISABLE_MIGRATE?

Copy link
Copy Markdown
Member Author

@deep1401 deep1401 Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its only 1. The comment is wrong. I'll fix. I'll actually just remove the variable altogether, we dont need it now. It was only for me

Copy link
Copy Markdown
Member Author

@deep1401 deep1401 Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

dadmobile
dadmobile requested changes Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this (NaN) while trying to view files? In case it matters the job failed (with the error I sent you) and confirmed in the network bar that it was trying to call http://localhost:8338/experiment/AWStest/jobs/NaN/files?subpath=:

Image

@dadmobile
Copy link
Copy Markdown
Member

dadmobile commented Mar 24, 2026

(Also same issue for provider logs as for file browser)

deep1401
deep1401 commented Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member Author

@deep1401 deep1401 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this (NaN) while trying to view files? In case it matters the job failed (with the error I sent you) and confirmed in the network bar that it was trying to call http://localhost:8338/experiment/AWStest/jobs/NaN/files?subpath=:

There were multiple parseInt(jobid) calls in the frontend. I fixed all of them. It should be fixed now, could check once more for the same job please?

@deep1401 deep1401 requested review from dadmobile March 24, 2026 18:59
dadmobile
dadmobile requested changes Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it is putting my old jobs at the top of the job history list?

Image

@deep1401
Copy link
Copy Markdown
Member Author

deep1401 commented Mar 24, 2026

It seems like it is putting my old jobs at the top of the job history list?

Image

I think thats probably because it tries to sort by id first, will fix

deep1401
deep1401 commented Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Member Author

@deep1401 deep1401 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it is putting my old jobs at the top of the job history list?

Okay so I fixed the sorting now and its two-fold. Jobs are sorted by either of created_at or started_at fields

dadmobile
dadmobile requested changes Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporarily rescinding approval while I try to figure out a better way to do the migration (or I don't and we just move forward).

dadmobile
dadmobile approved these changes Mar 25, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with team. G2G.

@deep1401 deep1401 merged commit c9caf2a into main Mar 25, 2026
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dadmobile dadmobile dadmobile approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@deep1401 @dadmobile