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.

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

Add examples/ into CI #474

Open
sbryngelson opened this issue Jun 19, 2024 · 6 comments · May be fixed by #525
Open

Add examples/ into CI #474

sbryngelson opened this issue Jun 19, 2024 · 6 comments · May be fixed by #525
Assignees
Labels
continuous-integration Continuous integration (CI) enhancement New feature or request good first issue Good for newcomers

Comments

@sbryngelson
Copy link
Member

Make each example case sufficiently small such that it can run on one processor for at least a few time steps. Then, add a few time steps of each example case to CI and set I/O to write at the end. Then check output for NaNs/Inftys.

We grow the size of the test suite this way, though it isn't as thorough a test as there will be no goldenfiles (though I suppose we could add them).

@sbryngelson sbryngelson added enhancement New feature or request good first issue Good for newcomers continuous-integration Continuous integration (CI) labels Jun 19, 2024
henryleberre added a commit to henryleberre/ChemMFC that referenced this issue Jul 4, 2024
@henryleberre
Copy link
Member

henryleberre commented Jul 4, 2024

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: https://github.com/henryleberre/ChemMFC/tree/henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

@sbryngelson
Copy link
Member Author

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

@sbryngelson
Copy link
Member Author

sbryngelson commented Jul 4, 2024

I see that I get NaN on my local machine in the timestep performance output for 1d_impact (haven't checked other cases).

 [ 99%]  Time step      198 of 201 @ t_step = 197
 [ 99%]  Time step      199 of 201 @ t_step = 198
 [100%]  Time step      200 of 201 @ t_step = 199
 Performance:                        NaN  ns/gp/eq/rhs

+-----------------------------------------------------------------------------------------------------------+
| Finished MFC:                                                                                             |
| * Total-time:    0s                                  * Exit Code:     0                                   |
| * End-time:      08:38:16                            * End-date:      08:38:16                            |
+-----------------------------------------------------------------------------------------------------------+

mfc: (venv) Exiting the Python virtual environment.

Also something about the new node20 requirements for github action checkout is killing our phoenix runner.

Note that this case doesn't appear to have failed, just the performance counter did (I think).

Update: I'm fixing node20 requirements on phoenix via pr #505

@henryleberre
Copy link
Member

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

I am not entirely opposed to this. My reasoning is that some cases require special attention and may not work out of the box with this kind of hacking.

For example, in the above-referenced commit, I hardcoded an exception for the "scaling" test case since it requires the user to input parameters. We could currently bypass this by changing the case to assume sensible defaults. For this case in particular, the best way to make it a test would be to pass arguments to to use a very small number of points (since it is a configurable parameter). You could argue that this configuration should be the default..

We have to see what @okBrian finds to make an educated decision.

@henryleberre
Copy link
Member

I see that I get NaN on my local machine in the timestep performance output for 1d_impact (haven't checked other cases).

 [ 99%]  Time step      198 of 201 @ t_step = 197
 [ 99%]  Time step      199 of 201 @ t_step = 198
 [100%]  Time step      200 of 201 @ t_step = 199
 Performance:                        NaN  ns/gp/eq/rhs

+-----------------------------------------------------------------------------------------------------------+
| Finished MFC:                                                                                             |
| * Total-time:    0s                                  * Exit Code:     0                                   |
| * End-time:      08:38:16                            * End-date:      08:38:16                            |
+-----------------------------------------------------------------------------------------------------------+

mfc: (venv) Exiting the Python virtual environment.

Also something about the new node20 requirements for github action checkout is killing our phoenix runner.

Note that this case doesn't appear to have failed, just the performance counter did (I think).

I see the same behavior in my chemistry runs. I haven't looked into it yet.

@sbryngelson
Copy link
Member Author

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

I am not entirely opposed to this. My reasoning is that some cases require special attention and may not work out of the box with this kind of hacking.

For example, in the above-referenced commit, I hardcoded an exception for the "scaling" test case since it requires the user to input parameters. We could currently bypass this by changing the case to assume sensible defaults. For this case in particular, the best way to make it a test would be to pass arguments to to use a very small number of points (since it is a configurable parameter). You could argue that this configuration should be the default..

We have to see what @okBrian finds to make an educated decision.

It seems reasonable that any case that takes command-line arguments should also have sensible defaults if no arguments are passed (presumably ones that run quickly on a laptop).

henryleberre added a commit to henryleberre/ChemMFC that referenced this issue Jul 10, 2024
henryleberre added a commit to henryleberre/ChemMFC that referenced this issue Jul 10, 2024
@okBrian okBrian linked a pull request Jul 17, 2024 that will close this issue
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration Continuous integration (CI) enhancement New feature or request good first issue Good for newcomers
Development

Successfully merging a pull request may close this issue.

3 participants