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

fix: rework podio options to allow specifying list, includes, and excludes #1323

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Mar 6, 2024

Briefly, what does this PR introduce?

This PR somewhat redefines the behavior of the option -Ppodio:output_include_collections in order to fix #300. There are now three similar options:

  • -Ppodio:output_collections specifies which output collections to write,
  • -Ppodio:output_include_collections specifies which output collections to include in addition to the default set,
  • -Ppodio:output_exclude_collections specifies which output collections to exclude from the default set.

The behavior is technically backwards compatible. Where previously -Ppodio:output_include_collections resulted in only those collections being added to the output, now those collections will be added to the default set.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No. More collections will be written when using -Ppodio:output_include_collections.

@wdconinc wdconinc linked an issue Mar 6, 2024 that may be closed by this pull request
@wdconinc wdconinc requested a review from veprbl March 6, 2024 15:04
@wdconinc wdconinc force-pushed the 300-podio-add-flag-to-save-additional-collections-to-the-output-file branch from 14500c9 to 5b24b75 Compare March 6, 2024 17:22
@wdconinc wdconinc enabled auto-merge March 6, 2024 20:48
@veprbl
Copy link
Member

veprbl commented Mar 28, 2024

I've been looking at this for a while now. I guess I have two concerns regarding this:

  1. This continues with the current practice of special treatment of empty value in podio:output_collections to instead initialize it with the default list. In practice, there is a case for not outputting any collections, when a plugin with custom JANA processor is loaded (this is done, for example, in EICrecon "benchmarks").
  2. This changes the meaning of podio:output_include_collections. The current user workflows are already using this parameter to improve performance of the reconstruction, or for other reasons. I think we could achieve your proposed naming, but, perhaps, it would be better to have some migration story first (e.g. deprecate the podio:output_include_collections before reintroducing it with a new meaning).

@wdconinc
Copy link
Contributor Author

Both reasonable concerns.

  1. Happy to remove the special condition. Do we need a boolean podio:output_copy_input_collections or so?
  2. We can deprecate it first. We'd need a command line argument deprecation strategy first: we could just print a warning (which won't be seen), or fail but accept a allow_deprecated_options for short term fixes. We can deprecate in 24.04, remove in 24.05, new syntax in 24.06.

@veprbl
Copy link
Member

veprbl commented Mar 29, 2024

Both reasonable concerns.

  1. Happy to remove the special condition. Do we need a boolean podio:output_copy_input_collections or so?

I haven't had a need to copy the input collections. It would be easy enough to analyze files in parallel because the events match.

  1. We can deprecate it first. We'd need a command line argument deprecation strategy first: we could just print a warning (which won't be seen), or fail but accept a allow_deprecated_options for short term fixes. We can deprecate in 24.04, remove in 24.05, new syntax in 24.06.

The pattern sounds like what I had in mind, we could do even slower. A boolean allow_deprecated_options has an issue that you can only require it once. We don't need compatibility tags for this one.

auto-merge was automatically disabled April 20, 2024 00:14

Merge queue setting changed

github-merge-queue bot pushed a commit that referenced this pull request Jun 3, 2024
…meter (#1466)

This is a follow up on a resolution from #1323

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [x] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No

### Does this PR change default behavior?
No
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PODIO, add flag to save additional collections to the output file
2 participants