Skip to content

Conversation

mafshari64
Copy link
Contributor

CI pass

All CI tests ran successfully with no failures.

@chillenzer Please review and let me know.

@mafshari64 mafshari64 force-pushed the CI_test_picmi_pypicongpu branch from e8148c1 to d816894 Compare August 29, 2025 16:03
@ikbuibui ikbuibui added the PICMI pypicongpu and picmi related label Sep 1, 2025
@ikbuibui
Copy link
Contributor

ikbuibui commented Sep 1, 2025

https://gitlab.com/hzdr/crp/picongpu/-/jobs/11201531056 CI fails in the pypicongpu compile test for LWFA

Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

First line already contains a breaking issue - please fix - after that I can continue reviewing

export PIC_SYSTEM_TEMPLATE_PATH=${PIC_SYSTEM_TEMPLATE_PATH:-"etc/picongpu/bash"}

export PICSRC=$HOME/src/picongpu
export PICSRC=/home/afshar87/afshari/simulation/picongpu
Copy link
Member

Choose a reason for hiding this comment

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

This is your specific code

Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

For now just a breaking issue:

  • logs have been uploaded

and a few questions:
Will continue reviewing after fixes and answers have been provided

Copy link
Member

Choose a reason for hiding this comment

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

This should not be included in a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will delete it. I think it is created during IC tests.

directory: str, optional
Directory inside simOutput for writing checkpoints (default: "checkpoints").
Directory to store checkpoint files. Default: "checkpoints".
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the useful information, that these directories are created in the simOutput directory?

file: str, optional
Relative or absolute fileset prefix for checkpoint files.
Base name for checkpoint files. Default: None.
Copy link
Member

Choose a reason for hiding this comment

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

Why this renaming?

tryRestart: bool, optional
If True, restart from the latest checkpoint if available, else start from scratch.
Attempt to restart from existing checkpoints. Default: None.
Copy link
Member

Choose a reason for hiding this comment

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

Why this in my opinion less clear description?

restartDirectory: str, optional
Directory inside simOutput containing checkpoints for restart (default: "checkpoints").
Directory to look for restart files. Default: None.
Copy link
Member

Choose a reason for hiding this comment

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

why remove the reference to this directory being inside simOutput?

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a reversion, unless you are actively changing something in this file please remove these changes.

Copy link
Member

Choose a reason for hiding this comment

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

This file should not be part of the PR

Copy link
Contributor Author

@mafshari64 mafshari64 Sep 1, 2025

Choose a reason for hiding this comment

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

I deleted test.log files and used the old definitions, which were clearer. I will wait for further comments and then push a new commit tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file touched at all, let alone what the actual changes are supposed to do?

@chillenzer
Copy link
Contributor

chillenzer commented Sep 3, 2025

Offline discussion: This PR will be split into one PR adding tests to PICMI/PyPIConGPU plugin implementations and one PR introducing the openPMD plugin. Further improvements are

OpenPMD plugin

Tests

  • Avoid testing already-tested functionality through different interfaces (like TimeStepSpec behaviour from within the plugin tests)
  • Introduce some infrastructure to unify and simplify the tests

Afterwards, we'll hopefully see more clearly and can actually start reviewing this contribution.

@chillenzer chillenzer marked this pull request as draft September 4, 2025 07:24
@PrometheusPi
Copy link
Member

@mafshari64 Your code has merge-conflicts. Please resolve.

@PrometheusPi
Copy link
Member

@mafshari64 The content of your last commit 51a0708 regarding the laser base definition is good but should not be added in a hidden way as part of your commit.
Please open it as a separate pull request.

@mafshari64
Copy link
Contributor Author

@mafshari64 The content of your last commit 51a0708 regarding the laser base definition is good but should not be added in a hidden way as part of your commit. Please open it as a separate pull request.

@PrometheusPi I plan to do it.

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

Successfully merging this pull request may close these issues.

5 participants