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

Implement stage-free delivery for project deliveries. #48

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Aratz
Copy link
Contributor

@Aratz Aratz commented Aug 9, 2023

What problems does this PR solve?

The delivery system has been switched to DDS. This program provides an option to upload files to a specific location. This makes the staging step unnecessary. Skipping this step would both reduce delivery times and simplify the code

The delivery service supports three delivery modes: project, runfolder, and runfolder for projects. This PR introduces a new delivery route for projects (the simplest case). The goal is to test if this works as expected and to gather feedback before we apply this change to the other (more complex) delivery modes. All former delivery routes remain available.

Some design questions to think about:

  • when using mover we needed an option to skip the delivery step (see this comment), but this is not strictly necessary anymore since:
    1. dds is available everywhere (and if not it is easily installable from PyPI)
    2. Whenever possible it is better to test the full delivery workflow before releasing to production
      As it is now, I have not implemented the skip_delivery option, is this something we want to keep?

Risk analysis:

  • This implementation relies on two new table in the database and uses them to determine if a project has been delivered before. There is a risk that project are delivered twice if different delivery routes are used. Maybe this could be fixed by looking into both tables, I can look into that and add to this PR if I manage to fix it.

How has the changes been tested?

In addition to automatic tests, has any manual testing been carried out? Unit tests and integration tests have been updated.

Reasons for careful code review

If any of the boxes below are checked, extra careful code review should be initiated.

  • This PR contains code that could remove data

Aratz added 21 commits March 6, 2023 10:06
This is because they are required by DDS when creating a project.
It seems this method confuses pytest too much and makes mocking
impossible.
OBS: there is a lot of code repetition in there and it's probably
possible to make these tests more concise by writting some helper
functions in the base class, but these tests are soon going to be
obsolete and are going to be removed once the new delivery endpoint is
released and the old ones are decommissioned.
This is in preparation for the big clean-up
This set a project as completed in the database
@Aratz Aratz requested a review from matrulda August 9, 2023 14:52
@Aratz Aratz self-assigned this Aug 9, 2023
Copy link
Member

@matrulda matrulda left a comment

Choose a reason for hiding this comment

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

Looks great! I left some comments for you.

Regarding skip_delivery I agree that it can be removed. When doing full integration testing / validation we want to deliver data to DDS. It is not ideal that we upload to the "production env", but since there is no "Staging" version of DDS, I don't really see how we would do it differently.

delivery/handlers/delivery_handlers.py Outdated Show resolved Hide resolved
delivery/models/db_models.py Outdated Show resolved Hide resolved
delivery/models/db_models.py Outdated Show resolved Hide resolved
delivery/models/db_models.py Outdated Show resolved Hide resolved
delivery/repositories/dds_repository.py Show resolved Hide resolved
delivery/models/db_models.py Outdated Show resolved Hide resolved
delivery/models/db_models.py Outdated Show resolved Hide resolved
@Aratz Aratz marked this pull request as ready for review August 22, 2023 08:10
@Aratz
Copy link
Contributor Author

Aratz commented Aug 22, 2023

Nice comments! Ready for the second round of review now. Then I think it's ready for merging and testing in staging after we update snpseq_packs.

@Aratz Aratz requested a review from matrulda August 22, 2023 08:19
Copy link
Member

@matrulda matrulda left a comment

Choose a reason for hiding this comment

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

Nice! ✔️

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.

2 participants