-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Add setup-conda action #46611
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
Add setup-conda action #46611
Conversation
@@ -0,0 +1,31 @@ | |||
name: Set up pandas |
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.
This action will eventually supersede the setup
action
cc @lithomas1 because you were worried about GHA capacity. I think this is fine now because of #46600. |
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.
One comment otherwise looks good!
looks ok. are we backporting these @mroeschke ? |
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.
Added some comments, but this is too big too be merged in my opinion. The CI is quite tricky, if we break things we can end up having errors that aren't caught in the CI (it happened several times in the past). Also, if things go wrong, we should be able to revert the windows or the mac changes, without having to also revert the others.
So, better go step by step. I'd start by adding the actions, when we're happy we add one of the builds (for mac or for windows), still keeping the one in azure for few days, until we're confident the new one is working fine, and catches the errors. And, then we open a new PR to remove the old one. Merging a PR this big can easily get us in trouble.
shell: bash -el {0} | ||
|
||
- name: Build Version | ||
run: pushd /tmp && python -c "import pandas; pandas.show_versions();" && popd |
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.
Why to execute the show_versions()
from /tmp
and not the current directory?
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.
Idk it's copied from other CI code
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.
I think it's save to remove the pushd
and popd
stuff.
Build Version
seems a bit confusing, Show dependency versions
or similar seems to be more descriptive of what this is doing.
This is a fair point. Might be good to just do macOS in 1 PR and Windows in another PR. Having them in separate workflows might be a good idea too since the paths and setup might differ a bit Would probably need backporting when ready. |
Added back Azure CI and enabled Windows only. I left the workflow name identical for now |
Can some please re-run the failed jobs? |
The easiest is usually to close and reopen the PR, and everything should re-run. Let me know of that doesn't work. |
python -m pip install -e . --no-build-isolation --no-use-pep517 --no-index | ||
time python setup.py build_ext -v -j $N_JOBS | ||
pip install -v -e . --no-build-isolation --no-use-pep517 --no-index | ||
shell: bash -el {0} |
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.
How is this shell different to the default?
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.
- It is a login shell (important for Conda to work)
- It fails if one of the commands fails, rather than just continuing.
shell: bash -el {0} | ||
env: | ||
# Cannot use parallel compilation on Windows, see https://github.com/pandas-dev/pandas/issues/30873 | ||
N_JOBS: ${{ runner.os == 'Windows' && 1 || 3 }} |
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.
Why 3 jobs if not windows? My understanding is that GItHub runners have 2 cores.
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.
macOS has 3, can change that back to 2 and change to 3 in a follow-up PR where we add macOS?
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.
Or we could just hardcode Windows 1, Linux 2, macOS 3 here?
shell: bash -el {0} | ||
|
||
- name: Build Version | ||
run: pushd /tmp && python -c "import pandas; pandas.show_versions();" && popd |
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.
I think it's save to remove the pushd
and popd
stuff.
Build Version
seems a bit confusing, Show dependency versions
or similar seems to be more descriptive of what this is doing.
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
Merged with main and reduced number of changes in this PR. It now mainly adds new actions that can be used by the other workflows in subsequent PRs, to deduplicate workflow code. |
Would it be possible to do Also, for |
And CircleCI. Shall I move docbuild to |
I think it would be okay to do it in this PR |
Moved |
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.
Awesome, great cleanup. LGTM
Awesome, thanks for the simplifying and the cleanup @jonashaag |
* Add run-tests and setup-pandas actions * Undo run-tests * Use setup-pandas in docbuild * Update docbuild-and-upload.yml * Update macos-windows.yml * Update docbuild-and-upload.yml * Update action.yml * Create action.yml * Delete .github/actions/setup-pandas directory * Review feedback * Update .github/workflows/macos-windows.yml Co-authored-by: Matthew Roeschke <[email protected]>
Smaller version of #46493 according to suggestion in #46493 (comment). Should be orthogonal to #46538.
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.