-
Notifications
You must be signed in to change notification settings - Fork 19
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
Simplify icon4py dependencies #839
Conversation
run only selected tests
still need to run all the tests, jenkins seems to be down.. |
|
@@ -181,13 +180,13 @@ def install(self, pkg, spec, prefix): | |||
elif self.spec.version == ver('=0.0.8'): | |||
build_dirs = [ | |||
'tools', 'model/atmosphere/dycore', | |||
'model/atmosphere/diffusion', 'model/driver', 'model/common/' | |||
'model/atmosphere/diffusion', 'model/common/' |
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.
good catch.
launch jenkins py-icon4py |
Could you also open an icon4py PR to undo these changes. Let's remove srun parts and keep the archiveArtifacts in both |
balfrin🟢 unit test
🟢 integration test
🔴 system test
WARNING: Serial tests did not run for system tests |
daint🟢 unit test
🟢 integration test
🔴 system test
WARNING: Serial tests did not run for system tests |
I have vacation this week. Will review it on Monday. A brief look at the changeset reveals that you break builds of older version. |
I discussed this also Abishek, in the meantime we have also merged a PR in icon4py which makes a lot of the dependencies optional, with this I can actually run the tests relevant for icon integration by using a Generally, we should have a discussion on versioning and releases. Currently in icon4py so much is going on and changing a lot, that I think it is reasonable to always integrate but we cannot guarantee to never break backward versions. That costs too much time. |
38bda3b
to
fd24c8b
Compare
I would actually withdraw all the version of py-icon4py that were running all greenline tests immediately. That is version |
@halungge Just let me know once you are ready for a review. |
remove patch file
ee15320
to
74f9478
Compare
launch jenkins py-icon4py |
tsa🟢 unit test
🟢 integration test
|
balfrin🟢 unit test
🟢 integration test
🔴 system test
WARNING: Serial tests did not run for system tests |
daint🟢 unit test
🟢 integration test
🔴 system test
WARNING: Serial tests did not run for system tests |
when='@0.0.8:', | ||
type=('build', 'run')) | ||
depends_on('py-pytest-mpi', when='@0.0.8:', type='build') | ||
def patch(self): |
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 am not sure if we want to name it patch
.
This has a special meaning in spackl, and it will be applied for every install.
I propose to give it a more meaningful name, i.e. like overwrite_pytest_ini
or something.
Also you can decorate it with something like
@on_package_attributes(run_tests=True)
to be exectuted only in case
--test=root
is set.
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.
yes that is precisely why I use patch
. I want it to run before the install
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.
If it is simply that you can use
@run_before("install")
decorator.
Just make sure it is a valid phase for python packages.
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.
But it's up to your flavour.
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.
Please also remove the failing test for removed version of icon4py in system_test.py
.
And run the test for icon too: launch jenkins icon
, since you change the icon-recipe as well
launch jenkins py-icon4py icon |
tsa🟢 unit test
|
balfrin🟢 unit test
|
daint🟢 unit test
|
Goes together with icon4py PR C2SM/icon4py#281
In order to simplify the spack build we only take into acccount what is relevant for the blueline (integration with icon-dsl)
--test=root
option unit