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

Working azure download flow #106

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Working azure download flow #106

wants to merge 25 commits into from

Conversation

zsusswein
Copy link
Collaborator

@zsusswein zsusswein commented Dec 5, 2024

This one turned into a bit of a mad dash to get everything ready for the demo. Everyone was able to run the pipeline locally and in Batch (success!) and I was able to run this branch in a shadow production run today. But now the branch has entirely too much content.

 I'd like to get this PR open and the functionality onto main rather than polishing a long-lived feature branch. If people are on board, that would mean liberally turning feedback into issues and circling back in a series of future PRs.

Thanks for bearing with me on this one folks!

Copy link

github-actions bot commented Dec 5, 2024

Thank you for your contribution @zsusswein 🚀! Your pkgdown-site is ready for download 👉 here 👈!
(The artifact expires on 2024-12-18T18:05:14Z. You can re-generate it by re-running the workflow here.)

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 46.34146% with 44 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
R/azure.R 14.28% 36 Missing ⚠️
R/pipeline.R 80.00% 8 Missing ⚠️
Additional details and impacted files

📢 Thoughts on this report? Let us know!

@kgostic
Copy link
Collaborator

kgostic commented Dec 9, 2024

Great job @zsusswein! Some notes from the demo. Some of these may be separate issues that we deal with later:

  • Roll back .env and config workarounds in demo instructions (after these are fixed)
  • Would be nice to add a mermaid diagram of all the relevant repos, environments, and microservices and how they connect
  • Would be nice to document in each step - from where is this step reading inputs and writing outputs (this would help with troubleshooting)

@zsusswein zsusswein mentioned this pull request Dec 10, 2024
3 tasks
@zsusswein zsusswein marked this pull request as ready for review December 11, 2024 17:24
@zsusswein zsusswein requested a review from kgostic December 11, 2024 17:26
@zsusswein
Copy link
Collaborator Author

zsusswein commented Dec 11, 2024

FYI @amondal2 I tagged you in case you want to take a look, but the azure stuff is kind of a mess. I'm planning on cleaning it up once this merges and tagging you and Nate to take a look.

Makefile Show resolved Hide resolved
azure/requirements.txt Outdated Show resolved Hide resolved
Comment on lines +54 to +59
task_configs = []
blobs = container_client.list_blobs()

for blob in blobs:
if blob.creation_time > two_mins_ago:
task_configs.append(blob.name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
task_configs = []
blobs = container_client.list_blobs()
for blob in blobs:
if blob.creation_time > two_mins_ago:
task_configs.append(blob.name)
task_configs: list[str] = [
b.name
for b in container_client.list_blobs()
if b.creation_time > two_mins_ago
]

Feel free to ignore. Just adding bc I like list comprehensions.

Dockerfile Outdated Show resolved Hide resolved
@zsusswein
Copy link
Collaborator Author

Some notes from the demo. Some of these may be separate issues that we deal with later:

Tracking in #116

@zsusswein zsusswein requested a review from athowes December 11, 2024 18:04
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
# Set up task on job
registry = os.environ["AZURE_CONTAINER_REGISTRY"]
task_container_settings = batchmodels.TaskContainerSettings(
image_name=registry + '/cfa-epinow2-pipeline:test-edit-azure-flow',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this break if we try and run it on a different branch?

docker run --mount type=bind,source=$(PWD),target=/cfa-epinow2-pipeline -it \
--env-file .env \
--rm $(REGISTRY)$(IMAGE_NAME):test-$(TAG) \
Rscript -e "CFAEpiNow2Pipeline::orchestrate_pipeline('$(CONFIG)', config_container = 'rt-epinow2-config', input_dir = '/cfa-epinow2-pipeline/input', output_dir = '/cfa-epinow2-pipeline', output_container = 'zs-test-pipeline-update')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Rscript -e "CFAEpiNow2Pipeline::orchestrate_pipeline('$(CONFIG)', config_container = 'rt-epinow2-config', input_dir = '/cfa-epinow2-pipeline/input', output_dir = '/cfa-epinow2-pipeline', output_container = 'zs-test-pipeline-update')"
Rscript -e "CFAEpiNow2Pipeline::orchestrate_pipeline('$(CONFIG)', config_container = 'rt-epinow2-config', input_dir = '/cfa-epinow2-pipeline/input', output_dir = '/', output_container = 'zs-test-pipeline-update')"

Use the same output dir as for the run-batch command. This way, the file paths in the metadata will always be the same as the those created by the batch command, which is desirable for maintaining consistency in output folder structure. However, this would require changing the bind mount above to mount to the container's root, instead of /cfa-epinow2-pipeline.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the point about consistency, but I think we should follow the pattern of this one, not run-batch. We want the local run to write into the bind mount, which this edit would prevent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what if we change the bind mount to be / inside the container for this make command?

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.

3 participants