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.

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

1025 - Add test_dispatch_to_batch #1042

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

avrohomgottlieb
Copy link
Contributor

@avrohomgottlieb avrohomgottlieb commented Jan 3, 2025

Issue Number

Closes #1025

Purpose/Implementation Notes

Add a test module for dispatch_to_batch which tests correct project querying and job submission.

Update ProjectFactory factory method to adequately handle the library and sample relationship.

Types of changes

What types of changes does your code introduce?

  • New feature (non-breaking change which adds functionality)

Functional tests

  • test_dispatch_to_batch
    • test_only_projects_without_computed_files_submitted
    • test_project_id_submission

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Screenshots

N/A

@avrohomgottlieb avrohomgottlieb marked this pull request as ready for review January 3, 2025 20:28
Copy link
Contributor

@davidsmejia davidsmejia left a comment

Choose a reason for hiding this comment

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

This looks mostly good, I think that we should also ensure that sample files are being generated until we are no longer supporting that use case.

Copy link
Contributor

@davidsmejia davidsmejia left a comment

Choose a reason for hiding this comment

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

I think the test_project_id_submission exposes opaqueness in how dispatch_to_batch handles existing computed files.

I think we should add a new arguments to the command:

  • --recreate-all: will call submit_jobs on all project or specified project in --project-id

Also let's update the docstring on the command to indicate that the default behavior will only dispatch batch jobs for all computed files where a project has none.

Comment on lines 175 to 178
@factory.post_generation
def add_sample_library_relation(self, create, extracted, **kwargs):
if not create:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a doc string that explains the many to many consideration here

self.assertIn(sample_no_files.scpca_id, submitted_sample_ids)

@patch("scpca_portal.management.commands.dispatch_to_batch.Command.submit_job")
def test_all_projects_have_computed_files(self, mock_submit_job):
Copy link
Contributor

Choose a reason for hiding this comment

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

assert that computed files exist here

mock_submit_job.assert_not_called()

@patch("scpca_portal.management.commands.dispatch_to_batch.Command.submit_job")
def test_project_id_submission(self, mock_submit_job):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_project_id_submission(self, mock_submit_job):
def test_project_id_argument(self, mock_submit_job):

@davidsmejia
Copy link
Contributor

Can you also add a log to the end of the dispatch to batch command that summarizes how many jobs were sent.

Comment on lines 25 to 28
If regenerate-all is passed, then all computed files are regenerated for all projects.
If a project-id is passed, and not combined with regenerate-all,
then computed files are only submitted for that specific project if it has no computed files.
If it is combined with regenerate-all, then computed files are regenerated regardless.
Copy link
Contributor

@davidsmejia davidsmejia Jan 16, 2025

Choose a reason for hiding this comment

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

I think this is more concise. Optionally, you can say that options may be combined.

Suggested change
If regenerate-all is passed, then all computed files are regenerated for all projects.
If a project-id is passed, and not combined with regenerate-all,
then computed files are only submitted for that specific project if it has no computed files.
If it is combined with regenerate-all, then computed files are regenerated regardless.
If regenerate-all is passed, then presence of existing computed files are ignored.
If a project id is passed then all other projects will be ignored.

Comment on lines 82 to 83
if project_id:
projects.filter(scpca_id=project_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This doenst look right to me, my understanding is that querysets are immutable. I would first try to make your tests fail with this implementation then fix this to overwrite the projects queryset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my mistake, you are correct. Addressed.

Copy link
Contributor

@davidsmejia davidsmejia left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test_dispatch_to_batch Module
2 participants