Skip to content

remove the duplication of fixtures#1186

Merged
yiluchen1066 merged 4 commits intomainfrom
refactor/consolidate-diffusion-exit-savepoint
Apr 2, 2026
Merged

remove the duplication of fixtures#1186
yiluchen1066 merged 4 commits intomainfrom
refactor/consolidate-diffusion-exit-savepoint

Conversation

@yiluchen1066
Copy link
Copy Markdown
Contributor

@yiluchen1066 yiluchen1066 commented Mar 31, 2026

  • Removed duplicate timeloop_diffusion_savepoint_exit (driver) and timeloop_diffusion_savepoint_exit_standalone (standalone_driver)
    fixtures that were identical to the shared savepoint_diffusion_exit in datatest.py
  • Added a small linit fixture alias in both driver and standalone_driver to bridge the parametrized timeloop_diffusion_linit_exit name
    to the linit name expected by the shared fixture

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run default

@yiluchen1066 yiluchen1066 requested a review from jcanton March 31, 2026 13:25
@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run distributed

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run default

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run distributed

@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Apr 1, 2026

I'm not sure but: there already is a linit fixture in model/testing/datatest.py why not use that directly instead of the alias?

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

I'm not sure but: there already is a linit fixture in model/testing/datatest.py why not use that directly instead of the alias?

the linit fixture in datatest.py always return False. The driver and the standalone driver tests need to override that value via @pytest.mark.parameterize("timeloop_diffusion_linit_exit", ...). So I added this alias fixture to do this override. Withouth the alias, savepoint_diffusion_exit would use linit=False regardless of what timeloop_diffusion_linit_exit is set in the parametrize decorator.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Mandatory Tests

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

  • cscs-ci run default
  • cscs-ci run distributed

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

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

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

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

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run default

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run distributed

@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Apr 2, 2026

I'm not sure but: there already is a linit fixture in model/testing/datatest.py why not use that directly instead of the alias?

the linit fixture in datatest.py always return False. The driver and the standalone driver tests need to override that value via @pytest.mark.parameterize("timeloop_diffusion_linit_exit", ...). So I added this alias fixture to do this override. Withouth the alias, savepoint_diffusion_exit would use linit=False regardless of what timeloop_diffusion_linit_exit is set in the parametrize decorator.

and it's not possible to change the linit fixture in datatest.py to return True/False?

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

I'm not sure but: there already is a linit fixture in model/testing/datatest.py why not use that directly instead of the alias?

the linit fixture in datatest.py always return False. The driver and the standalone driver tests need to override that value via @pytest.mark.parameterize("timeloop_diffusion_linit_exit", ...). So I added this alias fixture to do this override. Withouth the alias, savepoint_diffusion_exit would use linit=False regardless of what timeloop_diffusion_linit_exit is set in the parametrize decorator.

and it's not possible to change the linit fixture in datatest.py to return True/False?

technically it could be made parametrizable, but I think the issue is that linit in datatest.py serves as a default for all diffusion tests. Most diffusion tests expect linit=False (a normal timestep), and only a few specific tests override it to True via @pytest.mark.parametrize("linit", [True]). If we changed the linit fixture in datatest.py to return both True and False, it would affect every test that depends on it

@yiluchen1066
Copy link
Copy Markdown
Contributor Author

cscs-ci run default

Copy link
Copy Markdown
Contributor

@jcanton jcanton left a comment

Choose a reason for hiding this comment

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

nice! thanks for the cleanup and clarifications!

@yiluchen1066 yiluchen1066 merged commit 18b6135 into main Apr 2, 2026
54 checks passed
@yiluchen1066 yiluchen1066 deleted the refactor/consolidate-diffusion-exit-savepoint branch April 2, 2026 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants