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.

Sign up for GitHub

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

Jump to bottom

Fix parallel diffusion tests with dace orchestration #572

Merged
merged 11 commits into from
Nov 20, 2024
Merged

Fix parallel diffusion tests with dace orchestration #572

merged 11 commits into from
Nov 20, 2024

Conversation

DropD
Copy link
Contributor

@DropD DropD commented Oct 23, 2024 •
edited
Loading

Make sure unused connectivities are not passed to the compiled sdfg as kwargs.

@DropD DropD requested a review from kotsaloscv October 23, 2024 14:14
@DropD DropD changed the title filter out unused connectivities Fix parallel diffusion tests with dace orchestration Oct 23, 2024
@DropD DropD force-pushed the fix-orchestration-cached-rerun branch from b7e9ffe to 5ffb372 Compare October 23, 2024 14:15
@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

launch jenkins spack

kotsaloscv
kotsaloscv requested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@kotsaloscv kotsaloscv left a comment

Choose a reason for hiding this comment

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

LGTM.
Since the diffusion instance fixture was created just for calling the clear cache function, could you please remove it and restore the corresponding tests to their original form?
Thanks!

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

Since the diffusion instance fixture was created just for calling the clear cache function, could you please remove it and restore the corresponding tests to their original form? Thanks!

Are you sure the fixture doesn't make the tests a little cleaner even so?

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Oct 23, 2024

launch jenkins spack

@kotsaloscv
Copy link
Collaborator

Since the diffusion instance fixture was created just for calling the clear cache function, could you please remove it and restore the corresponding tests to their original form? Thanks!

Are you sure the fixture doesn't make the tests a little cleaner even so?

While it is a nice feature, the code we are replacing is quite minimal, so I would favour to go back to the original setup, just for the sake of uniformity with the rest of the tests.

kotsaloscv
kotsaloscv reviewed Oct 23, 2024
View reviewed changes
model/atmosphere/diffusion/tests/diffusion_tests/test_diffusion.py Outdated
@@ -382,7 +383,8 @@ def test_run_diffusion_single_step(
config = construct_diffusion_config(experiment, ndyn_substeps)
additional_parameters = diffusion.DiffusionParams(config)

diffusion_granule = diffusion_instance # the fixture makes sure that the orchestrator cache is cleared properly between pytest runs -if applicable-
exchange = definitions.create_exchange(processor_props, decomposition_info)
Copy link
Collaborator

@kotsaloscv kotsaloscv Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the non-MPI tests, I would do the following:

diffusion.Diffusion(backend=backend)

and let Diuffusion use the default exchange object.
The same comment applies to the other places as well.

Copy link
Collaborator

@kotsaloscv kotsaloscv Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the processor_props,decomposition_info are not needed. Because for these tests we do not need to pass an exchange object, i.e. the default one is ok.
The same comment applies to the other tests in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@DropD DropD force-pushed the fix-orchestration-cached-rerun branch from 3cbd48d to 3f04d3b Compare October 25, 2024 08:21
kotsaloscv
kotsaloscv approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@kotsaloscv kotsaloscv left a comment

Choose a reason for hiding this comment

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

LGTM

@kotsaloscv
Copy link
Collaborator

cscs-ci run default

@kotsaloscv
Copy link
Collaborator

launch jenkins spack

@kotsaloscv
Copy link
Collaborator

cscs-ci run dace

@halungge
Copy link
Contributor

halungge commented Nov 5, 2024

Can we merge this?

@kotsaloscv
Copy link
Collaborator

Can we merge this?

It is ready to be merged. The CI works again?

@kotsaloscv kotsaloscv closed this Nov 13, 2024
@kotsaloscv kotsaloscv reopened this Nov 13, 2024
@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

launch jenkins spack

@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

launch jenkins spack

@DropD
Copy link
Contributor Author

DropD commented Nov 13, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Nov 14, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 14, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Nov 14, 2024

launch jenkins spack

@DropD
Copy link
Contributor Author

DropD commented Nov 15, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 15, 2024

launch jenkins spack

@DropD
Copy link
Contributor Author

DropD commented Nov 15, 2024

cscs-ci run dace

@DropD
Copy link
Contributor Author

DropD commented Nov 18, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 18, 2024

launch jenkins spack

@DropD
Copy link
Contributor Author

DropD commented Nov 18, 2024

cscs-ci run default

@github-actions GitHub Actions
Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

For more detailed information please look at CI in the EXCLAIM universe.

@DropD
Copy link
Contributor Author

DropD commented Nov 19, 2024

cscs-ci run default

@DropD
Copy link
Contributor Author

DropD commented Nov 19, 2024

launch jenkins spack

@DropD DropD merged commit df0c34a into main Nov 20, 2024
4 of 5 checks passed
@DropD DropD deleted the fix-orchestration-cached-rerun branch November 20, 2024 08:00
jcanton pushed a commit that referenced this pull request Nov 20, 2024
@DropD @jcanton
Fix parallel diffusion tests with dace orchestration (#572)
f421d22 
* filter out unused connectivities
* remove diffusion_instance fixture
* update member blacklist in Diffusion.orchestration_uid
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kotsaloscv kotsaloscv kotsaloscv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DropD @kotsaloscv @halungge