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

Work with nate to improve the makefile #96

Merged
merged 17 commits into from
Nov 20, 2024

Conversation

gvegayon
Copy link
Member

This pull request includes updates to the Docker setup and documentation for building and running container images. The most important changes include updating the Dockerfile-dependencies, modifying the Makefile to add new targets and environment variables, and adding a new section in the README.md to document the container images and their usage.

Changes to Docker setup:

  • Dockerfile-dependencies: Updated the base image reference to include the full path docker.io/rocker/geospatial:4.4.1.

  • Makefile: Added new targets (deps, tag, interactive) and environment variables (TAG, REGISTRY) for building and managing container images.

Documentation updates:

  • README.md: Added a new section to document the container images, including instructions for building and running the images locally and using the Makefile targets.

@natemcintosh
Copy link
Collaborator

I'll wait for Zack to give the ok, but I think is otherwise ready to go

Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
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.

Sorry meant to post this comment with the review.

I like all the functionality that's here. A couple of additional things to think through:

  1. Can you add a pull recipe to get the image from ACR? We have secrets in key vault accessible in the NNH cli if they're needed.
  2. Can you think about the pros/cons of defaulting to podman locally and docker in CI? I'm a little uncomfortable with it, but want to talk about any benefits you may be seeing.

@natemcintosh
Copy link
Collaborator

I pushed a few changes. One was to replace the -v format for bind mounting with the --mount format; slightly longer, but also more explicit

@natemcintosh
Copy link
Collaborator

natemcintosh commented Nov 20, 2024

Can you add a pull recipe to get the image from ACR? We have secrets in key vault accessible in the NNH cli if they're needed.

Yea, are you thinking just the deps image? The main image? both?

Can you think about the pros/cons of defaulting to podman locally and docker in CI? I'm a little uncomfortable with it, but want to talk about any benefits you may be seeing.

Hmmm, we might have to ask @gvegayon for his experience with this so far. George, you have used this pattern of using podman locally and docker in CI before, correct? In my personal experience so far, it has been fine.

Laying out a few scenarios I can imagine:

  1. Best case scenario. No issues at all, everything runs fine forever.
  2. Meh case scenario. We notice some differences fairly quickly, and change the default in the Makefile to docker.
  3. Worst case scenario. There are near invisible differences in the images and/or behavior of the two. This goes on for some time before we notice the differences. This seems unlikely, but perhaps not impossible.
    1. In the best case for this scenario, our production runs were always done using image built in CI with docker, so all of our production runs are consistent. We discovered the issue because our local/prod runs weren't matching perfectly. We change the Makefile to use docker.
    2. In the worst case for this scenario, we had some production days where we had to make some quick bug fixes locally, and then pushed our podman-built image to be run, and the differences make it into our production runs.

Scenario 3ii seems highly unlikely, but would be a huge PITA. I'd probably put that scenario at a "Medium" on the risk matrix: rare, but critical in severity. Since the fix for this would be to simply use docker in the Makefile, I think we should go with that.

@zsusswein
Copy link
Collaborator

Yea, are you thinking just the deps image? The main image? both?

For sure the deps image -- maybe both?

Meh case scenario. We notice some differences fairly quickly, and change the default in the Makefile to docker.

This has been my experience. I had to work around podman being unable to access ACR without a password login. Which we can now do (https://github.com/cdcent/nnh-cli-tools/pull/9). Not a big deal, but hitting a difference almost immediately doesn't make me confident it's truly a "drop in replacement"

@zsusswein zsusswein self-requested a review November 20, 2024 18:48
Makefile Outdated Show resolved Hide resolved
@natemcintosh natemcintosh merged commit 88a0168 into zs-pipeline Nov 20, 2024
2 checks passed
@natemcintosh natemcintosh deleted the zs-pipeline-gvegayon-patch branch November 20, 2024 19:52
zsusswein added a commit that referenced this pull request Nov 22, 2024
* First draft of pipeline runner

* Fix logging

* Basic running end to end test

* Passing end-to-end test

But not THE end-to-end test

* Document the pipelining functions

* Turn off docs build in Image

Which was breaking because of missing TeX support to render the dir
structure in the new docs.

* Only use one core on CI

* Bump NEWs

* Hit exclusions in test

* Add test for missing stan keys error

* Fix helper

* Hit all the branches in the integration tests

* Work with nate to improve the makefile (#96)

* First draft of pipeline runner

* Fix logging

* Basic running end to end test

* Passing end-to-end test

But not THE end-to-end test

* Document the pipelining functions

* Turn off docs build in Image

Which was breaking because of missing TeX support to render the dir
structure in the new docs.

* Only use one core on CI

* Bump NEWs

* Work with nate to improve the makefile (it works)

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fixes for quality of life updates

* decide to swap to docker after conversation

* added pull command for the deps image

* swap fully to docker to remove uncertainty

---------

Co-authored-by: Zachary Susswein <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nate McIntosh <[email protected]>

* Whitespace

* Use names to call params in error message

* Move files written check into a helper

* Update pipeline wrapper func names

* Clarify NEWS

* Clarify config reqs in roxygen comment

---------

Co-authored-by: George G. Vega Yon <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Nate McIntosh <[email protected]>
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