-
Notifications
You must be signed in to change notification settings - Fork 16
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
make rapids-configure-conda-channels idempotent #104
make rapids-configure-conda-channels idempotent #104
Conversation
if [[ "${in_config}" == "true" ]]; then | ||
conda config --system --remove channels "${channel_id}" | ||
else | ||
echo "[rapids-configure-conda-channels] channel '${channel_id}' not found via 'conda config --get channels'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposing adding this so we have a chance of catching the "there was a typo in the channel name" case.
In theory this should never show up in CI logs unless there's a problem, since this script should only be run once during CI jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about simplifying all of this into just:
conda config --system --remove channels rapidsai || true
The || true
part will prevent it from exiting with a non-zero code if the channel doesn't exist.
And the standard conda error that you shared below should still print:
CondaKeyError: 'channels': 'rapidsai' is not in the 'channels' key of the config file
Thanks for reviewing! I'm proposing not
I'll defer to you. If you think the added complexity in the current state of this PR isn't worth the protection against those types of issues, I'll revert out all the |
@ajschmidt8 thoughts? Just noticed that this PR has been sitting for a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with some optional comments/suggestions
also, i noticed this script is missing our standard |
…slamb/gha-tools into idempotent-configure-conda-channels
also, i noticed this script is missing our standard set -euo pipefail. not sure if it's worth adding in this PR. I just added this in f1464ce Tested my new changes following the "How I tested this" steps in the description of the PR, all seemed to still work the expected way. I'll wait til you have another chance to review before I merge this @ajschmidt8 |
Thanks again for the thorough review and for teaching me some new |
While testing some RAPIDS projects, I've found it helpful to do something like this to test
conda builds
.(example: rapidsai/build-planning#54 (comment))
In that interactive case, it's useful to be able to modify the source of the project and directly run that
ci/build_*
script again.Many of the RAPIDS projects' build scripts include a call to
rapids-configure-conda-channels
near the beginning. (GitHub search. That interrupts this workflow because that script is not idempotent... it raises an exception if asked to remove a channel that isn't currently in the conda configuration.This proposes changing the behavior of that tool:
{channel}
from the conda configuration, raise an error if{channel}
isn't present there{channel}
exists in the conda configuration, remove it, otherwise warn that it already doesn't exist thereNotes for Reviewers
I can think of 2 reasons we might decide not to merge this:
rapidz
(note thez
) would result in a CI failure onmain
but just a log message on this branchI think these risks are worthwhile in exchange for removing some friction from local development on RAPIDS projects. I'm especially not too worried about the typo problem, because the channels this tool references are string literals within the tool, not coming through other configuration that's customized by projects.
How I tested this
in a container, using one of the images we use when building wheels in CI (click me)
From the root of this repo, on this branch.
docker run \ --rm \ -v $(pwd):/opt/gha-tools \ -w /opt/gha-tools \ -it rapidsai/ci-conda:cuda12.2.2-ubuntu22.04-py3.9-amd64 \ bash
Checked the current set of configured channels.
That printed the following.
Ran the script.
Saw this in its logs
Checked the channels again.
As expected,
rapidsai
was removed.Ran the script another 5 times, just to prove that it was really idempotent.
All of those succeeded, and the expected warning was issued.
Repeating this same process on
main
, the second call fails like this: