Skip to content
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

Refactor job creation code #3465

Merged
merged 78 commits into from
Oct 7, 2024
Merged

Refactor job creation code #3465

merged 78 commits into from
Oct 7, 2024

Conversation

amickan
Copy link
Contributor

@amickan amickan commented Aug 2, 2024 •
edited
Loading

This PR refactors the job creation code. Jobs, archive items and display sets now all use the same creation and validation logic.

Errors from asynchronous input validation now get caught and are reported back to the user as error message on the Job object. The job is then cancelled and not executed.

This PR also enables selecting existing images and files for jobs through the UI and enables re-using CIVs for existing values if possible. The same holds partially for the API view as well. It is not yet possible to create a job through the API with an existing non-image file. This was out of scope for this PR (https://github.com/DIAGNijmegen/rse-roadmap/issues/335#issuecomment-2304484574)

Part of https://github.com/DIAGNijmegen/rse-roadmap/issues/335

Closes #3139
Closes #3325
Closes #3565
Partially addresses #3368

@amickan amickan marked this pull request as ready for review August 23, 2024 14:53
@amickan amickan requested a review from jmsmkn as a code owner August 23, 2024 14:53
@amickan amickan removed their assignment Aug 23, 2024
jmsmkn
jmsmkn reviewed Aug 27, 2024
View reviewed changes
app/grandchallenge/algorithms/forms.py Show resolved Hide resolved
app/grandchallenge/algorithms/models.py Outdated Show resolved Hide resolved
app/grandchallenge/algorithms/models.py Outdated Show resolved Hide resolved
app/grandchallenge/algorithms/models.py Outdated Show resolved Hide resolved
app/grandchallenge/algorithms/models.py Outdated Show resolved Hide resolved
app/grandchallenge/components/tasks.py Outdated Show resolved Hide resolved
app/grandchallenge/components/tasks.py Outdated Show resolved Hide resolved
app/grandchallenge/components/views.py Outdated Show resolved Hide resolved
app/tests/algorithms_tests/test_api.py Outdated Show resolved Hide resolved
app/tests/algorithms_tests/test_forms.py Outdated Show resolved Hide resolved
@amickan
Copy link
Contributor Author

amickan commented Sep 23, 2024

This PR is ready for another round of review.

@amickan amickan assigned jmsmkn and unassigned amickan Sep 23, 2024
@jmsmkn
Copy link
Member

jmsmkn commented Sep 25, 2024

Sorry I've not reviewed this - been ill and not been able to concentrate on it properly so just doing easy work (MarkDown conversion).

@amickan
Copy link
Contributor Author

amickan commented Sep 25, 2024

Sorry I've not reviewed this - been ill and not been able to concentrate on it properly so just doing easy work (MarkDown conversion).

Get well soon, James!

@pkcakeout
Copy link
Contributor

pkcakeout commented Sep 25, 2024 •
edited
Loading

Beterschap James! The sniffle-time is upon us!

@amickan amickan mentioned this pull request Sep 26, 2024
Closed
jmsmkn
jmsmkn reviewed Oct 1, 2024
View reviewed changes
Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

Getting much easier to follow now, just some small points.

app/grandchallenge/profiles/models.py Outdated Show resolved Hide resolved
app/grandchallenge/profiles/models.py Outdated Show resolved Hide resolved
app/grandchallenge/cases/models.py Show resolved Hide resolved
app/grandchallenge/components/forms.py Outdated Show resolved Hide resolved
app/grandchallenge/components/form_fields.py Outdated Show resolved Hide resolved
app/grandchallenge/components/tasks.py Outdated Show resolved Hide resolved
@jmsmkn jmsmkn assigned amickan and unassigned jmsmkn Oct 2, 2024
@amickan amickan removed their assignment Oct 3, 2024
jmsmkn
jmsmkn approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

Epic work, well done!

@amickan
Copy link
Contributor Author

amickan commented Oct 3, 2024

Epic work, well done!

It only took 219 comments... Thanks for the help and review!

@jmsmkn
Copy link
Member

jmsmkn commented Oct 3, 2024 •
edited
Loading

Indeed, this is the 7th largest PR we've had, and the largest one that doesn't add/remove any vendored JS:

  1. Vendor cdnjs files: 173767 lines changed
  2. Consolidate management of vendored JS: 74266 lines changed
  3. Vendor uppy: 41626 lines changed
  4. Remove isolated viewers: 40311 lines changed
  5. Replace jsdelivr and datatables CDN : 33030 lines changed
  6. Add Cornerstone3D viewer for GPU enabled clientside viewing of volumes: 5116 lines changed
  7. Refactor job creation code: 5028 lines changed
  8. Remove the Retina API: 4291 lines changed
  9. Workstation annotation style options: 4260 lines changed
  10. Adds global-bundle.pem: 3054 lines changed
  11. Remove unused apps: 2871 lines changed
  12. Remove products app: 2837 lines changed
  13. Add Direct Messages: 2607 lines changed
  14. Deprecate images: 2445 lines changed
  15. Introduce TEXT-type answers: 2095 lines changed
  16. Update dependencies: 2082 lines changed
  17. Add ellipse drawing: 1929 lines changed
  18. Add prettier to unify formatting of js files: 1903 lines changed
  19. External evaluation phases: 1890 lines changed
  20. Add 3 point angle annotation type: 1888 lines changed
  21. Add SageMaker Training Backend: 1861 lines changed
  22. Groundtruth seperate from method container: 1859 lines changed
  23. Remove multi and single line text question types: 1848 lines changed
  24. Add Amazon SageMaker Batch backend: 1819 lines changed
  25. Remove inline JS: 1778 lines changed

@amickan amickan merged commit 2e92eeb into main Oct 7, 2024
8 checks passed
@amickan amickan deleted the job_refactoring branch October 7, 2024 06:34
@jmsmkn jmsmkn mentioned this pull request Oct 8, 2024
Closed
@jmsmkn jmsmkn mentioned this pull request Nov 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pkcakeout pkcakeout pkcakeout left review comments

@jmsmkn jmsmkn jmsmkn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@amickan @jmsmkn @pkcakeout