-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
CI: move prerelease_deps_coverage_64bit_blas
to GitHub actions.
#15958
Conversation
0df322c
to
8e87a46
Compare
@tupui I think you want to set this job up on your own fork first. That is by far the best way to iteratively work on CI jobs; only when it works on your own fork you want to switch to a PR here. |
ok sorry for the noise+CI |
See tupui#6 for discussions. |
CI failure is not related. (I opened an issue for that.) |
cc @tirthasheshpatel since you also looked at coverage, this might interest you. |
@rgommers @AnirudhDagar anything else I should do here? |
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 looks like a big improvment over azure: the CI time has reduced to 39 minutes from 60+ minutes on Azure. I am OK to merge this as is. The comments I have left can be addressed in a follow-up. Let me know if you want to address those here @tupui. Otherwise I think this is good to merge.
Thanks @tirthasheshpatel! I am fine getting this in since it would make the CI happy again. But I know very little about the some build deps as you noted. I would let @rgommers have a look/merge. |
Sure! I looked at the job. Other than caching, I can say that it is exactly equivalent to the azure job. So, I am +1 to merge this. Maybe, @HarshCasper can confirm if caching is done correctly here. |
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.
Thanks @tupui. There's a few minor things to fix, but overall this looks good. It does drop the ILP64 test completely though, so I'm not sure we want to merge this before we add that support back. We missed that when saying this job should be moved to Meson I think.
That decision depends on whether we can easily fix the current Azure job that's timing out I'd say. Does adding the one test skip to that job fix things there?
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.
Thanks @rgommers
It does drop the ILP64 test completely though, so I'm not sure we want to merge this before we add that support back. We missed that when saying this job should be moved to Meson I think.
My understanding was that ILP64 was just for the windows job. I don't see this elsewhere in the Azure config.
That decision depends on whether we can easily fix the current Azure job that's timing out I'd say. Does adding the one test skip to that job fix things there?
You mean, stay on Azure and just skip the test? I can try in another PR if you want.
I've bumped the milestone based on the feedback above, and to keep the "review queue" sane for branching. Sounds like we might be "ok" with skipping that test before we branch, if that's desired. |
Thanks Tyler. I think it's fine to leave as it is if it does not block the release. Since we updated the timeout on Azure it's mostly passing now. But it would just take me 2 seconds to make a PR with that so let me know if you want me to do it. |
So we're back to this PR still being open before another release/branch cutting. Looking through some comments:
cc: @eli-schwartz @rgommers maybe |
@rgommers has been working on an implementation of that in mesonbuild/meson#10921 but it's still marked as WIP. If he has more time to work on it in the next two or three weeks, then we might be able to get it merged inside the current Meson release cycle (currently projected to graduate to RC on December 9, which I guess is after SciPy's current cycle completes). |
I would like to work on ILP64 support in Meson soon again - right after getting the initial NumPy Meson build merged into NumPy |
@tupui I'm going to rebase on main and start pushing to your feature branch. |
[skip azp] [skip circle]
@rgommers @tupui this should be in a good state now. I squashed the commits to make history a bit cleaner.
|
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.
Thanks @andyfaff, this is looking pretty good, some minor comments to clean things up a bit more. The "avoid LD_LIBRARY_PATH
" one is the only substantial one.
|
||
- name: Test SciPy | ||
run: | | ||
export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH |
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.
We shouldn't ever need this, that makes me think something is wrong. I guess that's because of the custom OpenBLAS location? Can you try export PKG_CONFIG_PATH=/usr/local/lib
instead?
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.
Ah wait, not awake yet - we're testing not building.
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.
Self-correcting again, we are building as well as testing. Then PKG_CONFIG_PATH
should work and is preferred.
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.
The build occurs on L324 that works without any issue.
Without LD_LIBRARY_PATH the test fails/errors because libopenblas can't be found by the dynamic loader. Does PKG_CONFIG_PATH help with that?
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.
Ah yes, there's python dev.py build
higher up. It doesn't quite make sense to me that the build works but import fails at test time - probably something else is wrong.
The way this works is that if a library is in a non-default path, the build should write RPATH entries for those non-default paths into the extension modules so things work at test time. PKG_CONFIG_PATH
helps only with discovering dependencies at build time. If that works but then afterwards a library path goes missing, that's a bug somewhere. LD_LIBRARY_PATH
is an anti-pattern that should never be needed. The only reason it's kinda okay (but still not great) in the wheel builds is that afterwards we run auditwheel
/delocate
to vendor the relevant shared libraries.
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.
We could merge this once CI finishes and clean it up later though, to get on with the Azure migration.
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.
CI was green after 283def4
The following commits just removed comments.
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.
Can we merge @rgommers?
Co-authored-by: Pamphile Roy <roy.pamphile@gmail.com>
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.
Part of #15814
For now I did not use any templating, first trying to run this correctly in the CI.