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

metadataFromLaserWakefield can not be cloned #4948

Open
PrometheusPi opened this issue Jun 19, 2024 · 7 comments
Open

metadataFromLaserWakefield can not be cloned #4948

PrometheusPi opened this issue Jun 19, 2024 · 7 comments
Assignees
Labels

Comments

@PrometheusPi
Copy link
Member

PrometheusPi commented Jun 19, 2024

The metadataFromLaserWakefield test can not be pic-createed. The reference file picongpu-metadata.json.reference is not copied (since it is in the root of the setup) and thus the ci.sh test fails.

This bug was discovered by @max-lehman14.

@PrometheusPi PrometheusPi added bug a bug in the project's code component: tests unit tests labels Jun 19, 2024
@chillenzer
Copy link
Contributor

Interesting! The problem seems rather to be that there is some inconsistency in how the tests are designed to be run. My assumption was that you'd simply execute bin/ci.sh from the root of the test. It does the pic-create internally. This also seems to be how KHI and openPMD-viewer do things. Others like the shadowgraphy test make the pic-create step part of the manual instructions in the README. Others again seem to assume this step implicitly although bin/ci.sh from the "source" directory would potentially be possible.

I agree that we should unify this. I'm not sure if this is technically a bug -- at least not a bug located in the metadataFromLaserWakefield test.

@PrometheusPi
Copy link
Member Author

I agree with you @chillenzer that this is an inconsistency and not a bug. I will remove the flag.

@PrometheusPi PrometheusPi removed the bug a bug in the project's code label Jun 19, 2024
@chillenzer
Copy link
Contributor

I think this is more of a question of semantics. From my perspective, it looks as follows:

  • pic-create is supposed to create usable simulation directories from pre-assembled examples, tests, etc. That works perfectly fine. The moment I'm writing this, pic-build is compiling a working LWFA example from a pic-created copy of metadataFromLaserWakefield.

  • Running a test is not the same as running a simulation and, hence, needs to set up different things like copying over reference data for comparison. This is not covered by pic-create.

I don't think it's an oversight in pic-create and I don't think it's a design issue in the tests. I think it's out of scope for pic-create to handle such things. This implies that running a test should not involve a manual pic-create as default workflow because that would mean that an arbitrary number of different additional manual steps would be necessary for each test. The details of how to set up things should be handled by the ci.sh script to ensure reproducibility and ergonomics. I don't want the others not to run my carefully crafted test just because they have to do 100 manual steps before they can do so. It should be a single command, easier to do it than not to do it.

This doesn't mean that pic-create is useless here. It will be used inside of ci.sh and it should still be possible to do a manual pic-create to set up a simulation directory from any test case for running a simulation (as opposed to running the test).

An alternative perspective could be that similar to the situation in some of the differently designed tests, the validation belongs in the bin directory and related files (like the data to validate against) should move there. Personally, I'd find it confusing to find data in a bin directory but that would align better with the strategy of using pic-create for running tests.

As a hot fix, one could also move the reference file into etc/picongpu. Any thoughts, @PrometheusPi?

@BrianMarre
Copy link
Member

@chillenzer Tapish and I ran into a similar problem with the atomicPhysicsCI test case, we decided to create a separate validation directory, this could be a possible solution for this issue.

@chillenzer
Copy link
Contributor

Could you please add a link?

@ikbuibui
Copy link
Contributor

Sorry, I was not pinged. Yes, we faced a similar issue, but in our case we used the "submit action" of tbg to copy over the extra files. It was a use-case specific workaround

@psychocoderHPC
Copy link
Member

please have a look into this PR #5158
If the data get moved to validation folder within the setup they will be copied.
To be copied on tbg call the setup must add the explicit copy to the cfg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants