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

DM-32992: Allow the use of Docker images in GAR #5

Merged
merged 6 commits into from May 25, 2022
Merged

Conversation

yesw2000
Copy link
Contributor

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@timj timj changed the title Tickets/dm-32992 to allow using lsst Docker images in GAR DM-32992: Allow the use of Docker images in GAR May 13, 2022
@yesw2000
Copy link
Contributor Author

Hi @MichelleGower ,

I updated the file config/bps_idf.yaml with the lsst Docker image location in GAR, and added a new rst file doc/changes/DM-32992.misc.rst with an explanatory example. Please help review.

Many thanks,

Shuwei

Copy link
Contributor

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

Shouldn't the yaml in python/lsst/ctrl/bps/panda/conf_example also be updated? (And since the plugin has been split into its own package maybe move the conf_example up into the main directory instead of it being hidden under python?)

And there are a couple comments about how to set it to use docker hub (I didn't test what I asked in the question, so please test it to make sure the empty string does actually work.)

config/bps_idf.yaml Show resolved Hide resolved
@@ -0,0 +1,10 @@

Introduced a new parameter **dockerImageLocation** in the PanDA IDF configuration yaml file to pull lsst release containes from **GAR (Google Artifact Registry)**. This parameter is trailed with **'/'**, so it could be used in *sw_image* path in the following example. And the *sw_image* will still refer to the **Docker hub**, if the parameter **dockerImageLocation** is not defined, to make the *sw_image* to back compatible with previous PanDA IDF configuration yaml files.
Copy link
Contributor

Choose a reason for hiding this comment

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

By putting dockerImageLocation in the predefined config/bps_idf.yaml, there's not a way to make it "not defined" other than copying the file and editing the value in the copy. Instead, can't the user override just that one value in their submit yaml with the empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added another example to use images from the Docker hub.
And changed the description:

if the parameter dockerImageLocation is empty or not defined, to make the sw_image backward compatible with previous PanDA IDF configuration yaml files.

That means that users can start with this syntax now without waiting for the update of the config/bps_idf.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added an example of using the prefix {dockerImageLocation} into the file python/lsst/ctrl/bps/panda/conf_example/pipelines_check_idf.yaml.

Yes, I did test it with an undefined prefix, and it did pull that image from the Docker hub.

Copy link
Contributor

@MichelleGower MichelleGower left a comment

Choose a reason for hiding this comment

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

Found a couple typos and made a request to make how to get Docker hub images consistent. Looks like the branch needs to be rebased (https://developer.lsst.io/work/flow.html?highlight=rebase#pushing-code). Merge approved after rebasing and fixing these couple things.


Introduced a new parameter **dockerImageLocation** in the PanDA IDF configuration yaml file to pull lsst release containes from **GAR (Google Artifact Registry)**. This parameter is trailed with **'/'**, so it could be used in *sw_image* path in the following example. And the *sw_image* will still refer to the **Docker hub**, if the parameter **dockerImageLocation** is empty or not defined, to make the *sw_image* backward compatible with previous PanDA IDF configuration yaml files.

In the user bps submition yaml file, just prepend this parameter to the sw_image path, that is:
Copy link
Contributor

Choose a reason for hiding this comment

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

containes -> containers
submition -> submission


Please note that there is no any extra character(s) between *{dockerImageLocation}* and *lsstsqre*.

In case you have to use images from the Docker hub instead, you just take out the prefix **{dockerImageLocation}** in the path, that is:
Copy link
Contributor

Choose a reason for hiding this comment

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

This works. I wasn't expecting it though because in the bps_idf.yaml file it mentions setting dockerImageLocation to the empty string. So I would make it consistent throughout.. either setting it to empty string or removing the prefix in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @MichelleGower ,

The users can not modify config/bps_idf.yaml, so they have to remove the prefix in the path in order to use images from the Docker hub for their individual jobs. While on the system-wide side, the prefix can be set empty or undefined to force all the users to pull images from the Docker hub.

@yesw2000 yesw2000 merged commit 8bdcd46 into main May 25, 2022
@yesw2000 yesw2000 deleted the tickets/DM-32992 branch May 25, 2022 00:42
@yesw2000 yesw2000 restored the tickets/DM-32992 branch May 25, 2022 00:54
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.

None yet

2 participants