picongpu icon indicating copy to clipboard operation
picongpu copied to clipboard

metadataFromLaserWakefield can not be cloned

Open PrometheusPi opened this issue 1 year ago • 7 comments

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 avatar Jun 19 '24 12:06 PrometheusPi

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.

chillenzer avatar Jun 19 '24 12:06 chillenzer

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

PrometheusPi avatar Jun 19 '24 18:06 PrometheusPi

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?

chillenzer avatar Sep 11 '24 12:09 chillenzer

@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.

BrianMarre avatar Sep 11 '24 15:09 BrianMarre

Could you please add a link?

chillenzer avatar Sep 11 '24 15:09 chillenzer

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

ikbuibui avatar Sep 13 '24 09:09 ikbuibui

please have a look into this PR https://github.com/ComputationalRadiationPhysics/picongpu/pull/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

psychocoderHPC avatar Oct 07 '24 12:10 psychocoderHPC