Skip to content

Hv issue719 job manager threaded job start #736

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 95 commits into from
Closed

Hv issue719 job manager threaded job start #736

wants to merge 95 commits into from

Conversation

@HansVRP
Copy link
Contributor

@HansVRP HansVRP commented Feb 19, 2025

No description provided.

@HansVRP HansVRP requested a review from soxofaan February 19, 2025 15:55
@HansVRP
Copy link
Contributor Author

HansVRP commented Feb 19, 2025

@soxofaan maybe good to rediscuss from this point (local unit tests are passing)

soxofaan
soxofaan reviewed Feb 20, 2025
View reviewed changes
Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some quick notes

@soxofaan
Copy link
Member

soxofaan commented Feb 20, 2025 •
edited
Loading

by the way, I don't think you had to create a new PR, you just could have continued on #730 by pushing to its feature branch (we now have three open PRs for this feature I think, which gets a bit messy)

@HansVRP HansVRP requested a review from soxofaan February 21, 2025 14:38
@HansVRP
Copy link
Contributor Author

HansVRP commented Feb 21, 2025

worked on your initial comments @soxofaan

currently checking why some unit tests are not passing

soxofaan
soxofaan reviewed Feb 24, 2025
View reviewed changes
@soxofaan
Copy link
Member

soxofaan commented Feb 24, 2025

@soxofaan I do not think the failing unit test in 3.11 are due to my changes?

indeed, that problem has been fixed on master by now

@HansVRP
Copy link
Contributor Author

HansVRP commented Feb 25, 2025

One issue I still see now and want to resolve is that currently launching jobs (to queue them for start) is still tied in to the backend load.

This means than when running at full capacity you will not already 'precreate jobs' which can instantly start running

@soxofaan
Copy link
Member

soxofaan commented Feb 25, 2025

when running at full capacity you will not already 'precreate jobs' which can instantly start running

I'm not sure that pre-creating jobs instead of on the fly like we currently do will make that big of a difference as the time to create a job is usually negligible compared to the required time to start a job. Or do you experience different timings?

soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 no need to split up db_updates in _process_threadworker_updates
013c925 
is not necessary and eliminates `job_db.read()` which is not part of JobDatabaseInterface
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Introduce JobDatabaseInterface.get_by_indices
972aa9d 
- make sure `persist` gets complete rows
- implement in both `FullDataFrameJobDatabase` and `STACAPIJobDatabase`

refs: #719, #736, #793
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 fix typing warnings on test_job_management.DummyTask
44c9ec1 
and remove old "sleep" reference
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Add changelog entry #719/#736
c0e86c3
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Retry threaded job start (POST) on 429 too many requests
e8bd191 
refs #719, #441, #736

Explicitly add retry config for 429 response on POST request (e.g. start job)
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 Code style cleanup
5b9c231 
to improve signal/noise ratio of PR

(using darker and ruff)
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 Code style cleanup
8256190 
using `ruff format`, `ruff check --fix --select F401,I001` and `isort`
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 various tweaks based on review notes
03d7f44 
- split `Task` hierarchy for better separation of concerns
- more tests for some basic classes
- clean up unused fixtures from tests
- DummyBackend: add setup_job_start_failure
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 finetune test_thread_worker
d22cee9 
- support waiting directly in process_futures
- switch to DummyBackend
- use more dummy tasks TestJobManagerWorkerThreadPool to simplify setup
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 addressing some review notes (exclude bearer from repr)
d062568 
and some code style tweaks
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 no need to split up db_updates in _process_threadworker_updates
43afe85 
is not necessary and eliminates `job_db.read()` which is not part of JobDatabaseInterface
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Introduce JobDatabaseInterface.get_by_indices
f0305ca 
- make sure `persist` gets complete rows
- implement in both `FullDataFrameJobDatabase` and `STACAPIJobDatabase`

refs: #719, #736, #793
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 fix typing warnings on test_job_management.DummyTask
19d9976 
and remove old "sleep" reference
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Add changelog entry #719/#736
bd90546
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Retry threaded job start (POST) on 429 too many requests
5e5e2d3 
refs #719, #441, #736

Explicitly add retry config for 429 response on POST request (e.g. start job)
@soxofaan
Copy link
Member

soxofaan commented Sep 9, 2025

FYI: as this is a very long running PR with messy history, I tried to rebase the feature branch of this PR to clean it up a bit (e.g. commit squashing) to improve the signal-noise ratio and created PR #806 . The diff is practically identical to #736 (except for one empty line).

The discussion can stay here (#736), but #806 will be merged in the end

soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 Code style cleanup
23454d4 
to improve signal/noise ratio of PR

(using darker and ruff)
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 Code style cleanup
1b6f6a2 
using `ruff format`, `ruff check --fix --select F401,I001` and `isort`
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 various tweaks based on review notes
af55fd6 
- split `Task` hierarchy for better separation of concerns
- more tests for some basic classes
- clean up unused fixtures from tests
- DummyBackend: add setup_job_start_failure
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 finetune test_thread_worker
b20bdc4 
- support waiting directly in process_futures
- switch to DummyBackend
- use more dummy tasks TestJobManagerWorkerThreadPool to simplify setup
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 addressing some review notes (exclude bearer from repr)
5e5de21 
and some code style tweaks
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 no need to split up db_updates in _process_threadworker_updates
ed28aaa 
is not necessary and eliminates `job_db.read()` which is not part of JobDatabaseInterface
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Introduce JobDatabaseInterface.get_by_indices
434854f 
- make sure `persist` gets complete rows
- implement in both `FullDataFrameJobDatabase` and `STACAPIJobDatabase`

refs: #719, #736, #793
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
PR #736 fix typing warnings on test_job_management.DummyTask
3f95dee 
and remove old "sleep" reference
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Add changelog entry #719/#736
9100672
soxofaan added a commit that referenced this pull request Sep 9, 2025
@soxofaan
Retry threaded job start (POST) on 429 too many requests
2753365 
refs #719, #441, #736

Explicitly add retry config for 429 response on POST request (e.g. start job)
@soxofaan soxofaan mentioned this pull request Sep 9, 2025
Open
@soxofaan
Copy link
Member

soxofaan commented Sep 9, 2025

ok, I decided to merge this (through PR #806) in 4171004 🥳

@soxofaan soxofaan closed this Sep 9, 2025
@soxofaan soxofaan deleted the hv_issue719-job-manager-threaded-job-start branch September 9, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soxofaan soxofaan soxofaan left review comments

Assignees

No one assigned

Labels

job manager

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JobManager: create & start in parallel

3 participants

@HansVRP @soxofaan