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

Should the new SIPNET test run on every PR or should it be scheduled on weekly basis #3184

Open
GandalfGwaihir opened this issue Jun 11, 2023 · 8 comments · May be fixed by #3183
Open

Should the new SIPNET test run on every PR or should it be scheduled on weekly basis #3184

GandalfGwaihir opened this issue Jun 11, 2023 · 8 comments · May be fixed by #3183

Comments

@GandalfGwaihir
Copy link
Collaborator

GandalfGwaihir commented Jun 11, 2023

Description

Is your feature request related to a problem? Please describe.

As part of GSoc Project, We are creating a workflow which will test the current SIPNET model against the full docker stack brought up by docker compose.
It is resource intensive as we pull all the images in the docker compose and build them.

The testing workflow can be found in the draft PR: #3183

Proposed Solution

So should we schedule it for weekly basis or let it run on every PR ?

I was of the opinion to run it weekly, but open to the suggestions from the community.

cc. @robkooper @mdietze @infotroph

@mdietze
Copy link
Member

mdietze commented Jun 12, 2023

Is it possible to set it to run daily but to skip if there's been no change to the code? If not, weekly is probably good. Would just want to make sure that the notification for the status goes somewhere that will actually have an impact. The only advantage of the PR option is that it blocks things until it is fixed. If a failed integration test just sends an email that gets ignored then its not doing much.

@GandalfGwaihir
Copy link
Collaborator Author

GandalfGwaihir commented Jun 12, 2023

I also wanted to point that this test would not be limited to just the sipnet model, but in future different models would be tested, for that I have added the matrix option where we can add different parameters like site_id, mode_id, etc to test against the docker stack, and all these tests can run in parallel.

So, I think that keeping it with the PR option might not be helpful.

Also can we go for the weekly option? Because When the PR's will get merged and the latest images will get uploaded, then we can pull the latest images and test against them.

Currently we make a GET request to display the output of the sipnet test

@mdietze
Copy link
Member

mdietze commented Jun 12, 2023

@RohanSasne with your proposal to test against a matrix of different models, sites, etc, I wanted to double check that you are UPDATING the existing suite of integration tests not REINVENTING that suite.

Also, for the specific test you linked to above, Niwot Ridge is not a tropical forest. There's nothing gained by testing site x PFT combinations that aren't plausible.

@GandalfGwaihir
Copy link
Collaborator Author

GandalfGwaihir commented Jun 12, 2023

Opps, I had used those parameters as those were mentioned in the documentation to test the workflow , Can you please provide me with the correct parameters to test the full run.

Does this file contain the correct parameter?

Edit:

I actually tried with the parameters mentioned in the file above and the test was successfull on my localhost, updating the latest changes on my PR, will update this comment with the link shortly

@GandalfGwaihir
Copy link
Collaborator Author

GandalfGwaihir commented Jun 12, 2023

Hey @mdietze, The parameters are now according to the file i Referred above, can you please have a look at the workflow now

@RohanSasne with your proposal to test against a matrix of different models, sites, etc, I wanted to double check that you are UPDATING the existing suite of integration tests not REINVENTING that suite.

The current use of matrix will create a new job altogether for every Matrix element(unique job for each test parameters) and it will also need to pull and build the whole docker stack again, if we do not want to build it again then a option would be to make a script file consisting of the cURL requests to run our models, and then we can just append different models in the script file, So should we proceed with that direction or the current matrix jobs are fine?

@mdietze
Copy link
Member

mdietze commented Jun 12, 2023

@RohanSasne Niwot ridge is a conifer site

Also, on the issue of a matrix of jobs, I'm not saying that you can't modify how things are done, I'm saying you can't duplicate how things are done. Having two different jobs running the same tests doesn't work. Having two different code bases do the same thing doesn't work. If there's pros/cons of different approaches it would be good to lay those out BEFORE you reinvent something

@GandalfGwaihir
Copy link
Collaborator Author

Agree with you sir.

The only benefit of running a matrix would be that we can save sometime to test full run when we add more models to it, but again it would be much more resource intensive, so for now I think that it's better to run a script file than having matrix.

So should I update the PR accordingly @mdietze ?

@GandalfGwaihir GandalfGwaihir linked a pull request Jun 14, 2023 that will close this issue
13 tasks
@GandalfGwaihir
Copy link
Collaborator Author

Hey, @mdietze ,

The PR is up for review, can you please have a look whenever you get free :)

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 a pull request may close this issue.

2 participants