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

Move creation of env.yaml outside the current directory #14476

Merged
merged 10 commits into from Dec 12, 2023

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Nov 22, 2023

Description

Moves the creation of the env.yaml file in build scripts to a temporary directory instead of the current directory.

During the CI builds, a env.yaml file is created for use with a mamba call later in the script. This is a problem when trying to reproduce CI locally when the launched docker container tries to create this file locally as root. If the user does not have root access (or equivalent permissions), the file is not created and the process aborts.

The same error occurs when creating the test_results directory when the RAPIDS_TEST_DIR is not set. In this case the script tries to create the test_results in the current working directory.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added bug Something isn't working 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Nov 22, 2023
@davidwendt davidwendt self-assigned this Nov 22, 2023
@github-actions github-actions bot added ci and removed libcudf Affects libcudf (C++/CUDA) code. labels Nov 22, 2023
@vyasr
Copy link
Contributor

vyasr commented Nov 22, 2023

Could we port this change to other repos as well? We have a new tool rapids-reviser that can help automate that change, @bdice could probably show you how to set this up (although scripting this change is slightly nontrivial relative to our usual since this isn't just a single sed replacement but also a new line addition).

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Dec 5, 2023
@davidwendt davidwendt marked this pull request as ready for review December 5, 2023 22:39
@davidwendt davidwendt requested a review from a team as a code owner December 5, 2023 22:39
ci/build_docs.sh Outdated Show resolved Hide resolved
@@ -6,12 +6,14 @@ set -euo pipefail
rapids-logger "Create test conda environment"
. /opt/conda/etc/profile.d/conda.sh

ENV_YAML_DIR="$(mktemp -d)"
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly necessary since these scripts almost always run in a container, but cleaning up temp directories isn't a bad idea.

Suggested change
ENV_YAML_DIR="$(mktemp -d)"
ENV_YAML_DIR="$(mktemp -d)"
trap 'rm -rf -- "$ENV_YAML_DIR"' EXIT

Copy link
Member

Choose a reason for hiding this comment

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

Aren't temp directories cleared on boot automatically? I'd vote to omit this extra line to keep things as succinct as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Plus like you mentioned, 99% of the time, these scripts are run in containers.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

thanks David

@vyasr
Copy link
Contributor

vyasr commented Dec 12, 2023

I'm fine with getting this merged ASAP, but could we open an issue to track #14476 (comment)?

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 303cb69 into rapidsai:branch-24.02 Dec 12, 2023
70 checks passed
@davidwendt davidwendt deleted the move-env-yaml branch December 12, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working ci non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants