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: avoid pip executable when upgrading pip in circleci #5057

Conversation

drh-determined-ai
Copy link
Contributor

@drh-determined-ai drh-determined-ai commented Sep 19, 2022

Description

fix: avoid pip executable when upgrading pip in circleci

Test Plan

CircleCI tests should succeed.

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.
  • If modifying /webui/react/src/shared/ verify make -C webui/react test-shared passes.

@cla-bot cla-bot bot added the cla-signed label Sep 19, 2022
@netlify
Copy link

netlify bot commented Sep 19, 2022

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 9e82011
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6328a40e30dc850008b96fc0

@netlify
Copy link

netlify bot commented Sep 19, 2022

Deploy Preview for storybook-det canceled.

Name Link
🔨 Latest commit 9e82011
🔍 Latest deploy log https://app.netlify.com/sites/storybook-det/deploys/6328a40e332956000c668968

@drh-determined-ai drh-determined-ai changed the title bugfix: replace pip with python -m pip bug: replace pip with python -m pip Sep 19, 2022
@drh-determined-ai drh-determined-ai changed the title bug: replace pip with python -m pip fix: replace pip with python -m pip in CircleCI Sep 19, 2022
@drh-determined-ai drh-determined-ai changed the title fix: replace pip with python -m pip in CircleCI fix: avoid pip executable when upgrading pip in circleci Sep 19, 2022
Copy link
Contributor

@mpkouznetsov mpkouznetsov left a comment

Choose a reason for hiding this comment

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

Does the trick!

Copy link
Member

@rb-determined-ai rb-determined-ai left a comment

Choose a reason for hiding this comment

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

If you are not sure what precisely causes only this pip call to fail (and only fail in the windows runner) I'd recommend a comment explaining which conditions require python -m pip "for reasons which are unclear" or something to that effect.

Otherwise it looks real weird to the next developer who goes and reads this code carefully.

I wouldn't probably waste more than 20 minutes trying to figure out why fixing only this line fixes the test, if you decide to go that route.

@drh-determined-ai drh-determined-ai merged commit f2fe163 into determined-ai:master Sep 19, 2022
@dannysauer
Copy link
Member

In the future, maybe try to split unrelated cosmetic fixes out from technical fixes; that mixing stuff up makes tracking down "what changed" harder.

@dannysauer dannysauer modified the milestones: 0.0.102, 0.19.4 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants