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

Fix the cmdlineargs tests on Buildkite #42118

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

DilumAluthge
Copy link
Member

This change is necessary to get the cmdlineargs tests passing on Buildkite.

Ref #41977

@DilumAluthge DilumAluthge added test This change adds or pertains to unit tests backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 3, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 4, 2021

Is buildkite forcing threads to be enabled? If so, that is a buildkite bug.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 4, 2021

Is buildkite forcing threads to be enabled? If so, that is a buildkite bug.

In our Buildkite setup, we have it set up to run with JULIA_NUM_THREADS=16.

But this isn't actually specific to Buildkite. If you do export JULIA_NUM_THREADS=4 locally, and then run the cmdlineargs tests, they will fail.

I think we want these tests to pass regardless of whether or not the user has set the JULIA_NUM_THREADS environment variable, right?

@DilumAluthge
Copy link
Member Author

An alternate approach would be to run these particular tests inside a withenv that sets JULIA_NUM_THREADS to nothing.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Sep 4, 2021

The other alternative is to fix the bug described in your MWE in #41977 (comment).

I have no idea how to fix that bug 😭, which is why I figured I'd patch the tests instead.

@DilumAluthge
Copy link
Member Author

@staticfloat @vtjnash Can I merge this?

As mentioned above, this PR is necessary to get Buildkite working.

But also, it's necessary to fix the cmdlineargs tests for any user that has e.g. export JULIA_NUM_THREADS=4 in their environment.

@staticfloat
Copy link
Sponsor Member

Is buildkite forcing threads to be enabled? If so, that is a buildkite bug.

We need a way to limit the number of threads Julia spawns with (since we run multiple runners on a giant 128-core system), so we set JULIA_NUM_THREADS as part of the environment.

I say let's merge this as-is, then revisit.

@DilumAluthge DilumAluthge merged commit e1669b6 into master Sep 6, 2021
@DilumAluthge DilumAluthge deleted the dpa/fix-cmdlineargs-tests-buildkite branch September 6, 2021 23:28
@DilumAluthge DilumAluthge added the domain:ci Continuous integration label Sep 6, 2021
KristofferC pushed a commit that referenced this pull request Sep 7, 2021
KristofferC pushed a commit that referenced this pull request Sep 8, 2021
KristofferC pushed a commit that referenced this pull request Sep 8, 2021
KristofferC pushed a commit that referenced this pull request Nov 11, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Nov 13, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ci Continuous integration test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants