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

Linking to container image via --template when creating AZ Batch pool #89

Merged
merged 27 commits into from
Nov 22, 2024

Conversation

gvegayon
Copy link
Member

@gvegayon gvegayon commented Oct 22, 2024

This pull request includes significant updates to the Azure Batch Pool setup process, including adding a new Python script for pool creation and configuration and updates to the workflow and dependencies.

Azure Batch Pool Setup Enhancements:

  • Added a new Python script azure/pool.py to handle Azure Batch Pool creation and configuration using a TOML configuration file. This script includes functions for creating containers, reading autoscale formulas, and setting up the batch pool with the necessary credentials and configurations.
  • Updated the workflow file .github/workflows/containers-and-az-pool.yaml to run the new Python script, replacing the previous direct Azure CLI commands. This includes steps for writing the configuration file, ensuring the Azure CLI is installed, and running the Python script. [1] [2] [3]

Documentation and Dependency Updates:

  • Updated NEWS.md to include a note about the new script for setting up the Azure Batch Pool.
  • Added new dependencies to azure/requirements.txt required by the new Python script, including azure-common, azure-core, azure-identity, azure-keyvault, azure-mgmt-batch, and toml.

@gvegayon gvegayon linked an issue Oct 22, 2024 that may be closed by this pull request
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!


🚨 Try these New Features:

Copy link

github-actions bot commented Nov 13, 2024

Thank you for your contribution @gvegayon 🚀! Your pkgdown-site is ready for download 👉 here 👈!
(The artifact expires on 2024-11-29T20:59:37Z. You can re-generate it by re-running the workflow here.)

@gvegayon
Copy link
Member Author

So we are getting closer @jkislin and @zsusswein. There's an issue with the generated toml file that is makes it fail during the read process:

    config = toml.load(config_file)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/toml/decoder.py", line 134, in load
    return loads(ffile.read(), _dict, decoder)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/toml/decoder.py", line 514, in loads
    raise TomlDecodeError(str(err), original, pos)
toml.decoder.TomlDecodeError: Found tokens after a closed string. Invalid TOML. (line 10 column 1 char 754)

I'll try to debug it tomorrow, but I think we are almost there!

@gvegayon gvegayon force-pushed the 59-link-built-pool-to-image-in-acr branch from e4660a3 to 1053ef0 Compare November 20, 2024 20:20
@gvegayon
Copy link
Member Author

I am getting errors on testthat b/c of duckdb. The issue is intermittent, so either we are missing a seed, or there's something else going on. https://github.com/CDCgov/cfa-epinow2-pipeline/actions/runs/11943535928/job/33293099360#step:3:309. I have had to restart the job many times in the past, with the second try working. Unsure what's the issue

attn @natemcintosh @zsusswein

@zsusswein
Copy link
Collaborator

zsusswein commented Nov 20, 2024

That's weird. I've seen an intermittently flaky test from DuckDB but it's rare and I thought I had resolved it.

Can you open an issue for it and I'll look into it?

EDIT: #97

@jkislin
Copy link
Contributor

jkislin commented Nov 21, 2024

@gvegayon @zsusswein do we want to merge this now?

@zsusswein
Copy link
Collaborator

Is the PR ready to go? I had been assuming it was waiting for something because it was still draft.

@gvegayon gvegayon marked this pull request as ready for review November 22, 2024 15:52
@gvegayon
Copy link
Member Author

Given that the tests pass, this is ready for review @zsusswein and @jkislin!

@zsusswein zsusswein self-requested a review November 22, 2024 16:09
Copy link
Collaborator

@zsusswein zsusswein left a comment

Choose a reason for hiding this comment

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

Great work George! Thanks for pushing this through.

Marking to-dos as I understand them here. I'll make these into issues for future fixes:

  • Clean up the config toml and distribute differently (and encrypted)
  • Move the python action into it's own docker container with its own dependencies than a requirements in this repo
  • Move this action over to the cfa actions repo
  • take out the ABS stuff
  • clean up the python code

If there's anything else can you make it into a new issue?

@zsusswein zsusswein enabled auto-merge (squash) November 22, 2024 20:52
@zsusswein zsusswein merged commit 54fec40 into main Nov 22, 2024
7 checks passed
@zsusswein zsusswein deleted the 59-link-built-pool-to-image-in-acr branch November 22, 2024 21:00
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.

Link built pool to image in ACR
3 participants