-
Notifications
You must be signed in to change notification settings - Fork 284
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
update scipy easyblock for scipy >= 1.9.0 to use meson/ninja #2862
update scipy easyblock for scipy >= 1.9.0 to use meson/ninja #2862
Conversation
figuring out the best way to ensure all the tests are run in a way they don't fail silently |
Test report by @jfgrimm |
This comment was marked as outdated.
This comment was marked as outdated.
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 2 (2 easyconfigs in total) |
test failures actually are present in old builds, they were just ignored (see #2237) |
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
easybuild/easyblocks/s/scipy.py
Outdated
extra_vars = ({ | ||
'enable_slow_tests': [False, "Run scipy test suite, including tests marked as slow", CUSTOM], | ||
# ignore tests by default to maintain behaviour in older easyconfigs | ||
'ignore_test_result': [True, "Run scipy test suite, but ignore result (only log)", CUSTOM], |
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 should set this to None
, and auto-enable for scipy
versions >= 1.9.0 unless it's explicitly set to False
or True
in easyconfig?
easybuild/easyblocks/s/scipy.py
Outdated
|
||
|
||
class EB_scipy(FortranPythonPackage): | ||
class EB_scipy(FortranPythonPackage, MesonNinja): |
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'm a bit surprised that using MesonNinja
works to install scipy
as an extension, since that generic easyblock doesn't derive from ExtensionEasyBlock
(which is sort of required for stuff you want to install both as extension and stand-alone).
That's probably because MesonNinja
is only used as "secondary" easyblock, next to FortranPythonPackage
, but to make this future-proof (there will come a time where deriving from FortranPythonPackage
is useless since the scipy versions we still support are all >= 1.9.0) we may need to be a bit more careful here (which could be as simple as changing MesonNinja
to derive from ExtensionEasyBlock
, rather than from EasyBlock
like it does now).
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.
so I had a bit of a closer look, and the key thing that makes this work as-is seems to be calling an __init__
(e.g. PythonPackages.__init__
) which derives from ExtensionEasyBlock
With the new changes, the meson build still makes use of PythonPackage
anyway (to get the python command, and for the sanity check), so now removing FortranPythonPackage
would not break anything.
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 8 out of 8 (8 easyconfigs in total) |
easybuild/easyblocks/s/scipy.py
Outdated
if self.cfg['ignore_test_result'] is None: | ||
self.cfg['ignore_test_result'] = True | ||
# ignore installopts inherited from FortranPythonPackage | ||
self.cfg['installopts'] = "" |
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 should at least also log the original installopts
value if we're going to do a hard reset 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.
updated to strip the ones inherited from PythonPackage, perhaps that's better?
From a build of easybuilders/easybuild-easyconfigs#16912 started at 11:19:59 - so with 016118a |
Co-authored-by: Simon Branford <[email protected]>
simplify logic to set default value for ignore_test_result in scipy easyblock
Test report by @jfgrimm Overview of tested easyconfigs (in order)
Build succeeded for 14 out of 14 (11 easyconfigs in total) |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 11 out of 11 (11 easyconfigs in total) |
(created using
eb --new-pr
)