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

Update runDataProcessing.cxx to allow for - and _ in (Process?)Configurable<bool> names #12771

Closed
wants to merge 1 commit into from

Conversation

cholmcc
Copy link
Contributor

@cholmcc cholmcc commented Feb 27, 2024

No description provided.

@cholmcc cholmcc requested a review from a team as a code owner February 27, 2024 10:01
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass
async-2023-pp-apass1
async-2022-pp-apass6
async-2022-pp-apass4
async-mc
async-data

@cholmcc
Copy link
Contributor Author

cholmcc commented Feb 27, 2024

build/O2/o2-dataflow-cs8 failure seems unrelated.

@alibuild
Copy link
Collaborator

alibuild commented Feb 27, 2024

Error while checking build/O2/fullCI for 7f4bcf3 at 2024-02-29 07:25:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'


## sw/BUILD/O2-full-system-test-latest/log
task timeout reached .. killing all processes


## sw/BUILD/o2checkcode-latest/log
--
========== List of errors found ==========
++ GRERR=0
++ grep -v clang-diagnostic-error error-log.txt
++ grep ' error:'
++ GRERR=1
++ [[ 1 == 0 ]]
++ mkdir -p /sw/INSTALLROOT/69888d193c896a6906a834795f31d2bdeb4133e9/slc8_x86-64/o2checkcode/1.0-local138/etc/modulefiles
++ cat
--

Full log here.

@ktf
Copy link
Member

ktf commented Feb 27, 2024

@aalkin I think you were saying privately that this happens because we do have some convention on the process options which is not respected? Can you comment here?

@aalkin
Copy link
Member

aalkin commented Feb 27, 2024

@ktf indeed, this code is only applied to process switches, and by default those inherit their names after the process function which cannot contain dashes due to them being C++ identifiers. However, this is not a strict requirement as long as the names are OK to be put in JSON.

@cholmcc could you please change the title to say ProcessConfigurable ?

@jgrosseo
Copy link
Collaborator

I am not against this PR but I don't understand why you, Christian, use instead of normal configurables the ones of process switches?

@cholmcc
Copy link
Contributor Author

cholmcc commented Feb 28, 2024

@ktf indeed, this code is only applied to process switches, and by default those inherit their names after the process function which cannot contain dashes due to them being C++ identifiers. However, this is not a strict requirement as long as the names are OK to be put in JSON.

I'm afraid that the above is not entirely correct. The code in question is also called for regular switches as far as I can tell.

Take the O2Physics:PWGMM/Rivet/Tasks/rivet.cxx workflow, which among other things defines regular (not process) boolean switches like hepmc-recenter. The code in question is called on these options too. That may not be the intention, but it is what happens, I believe.

@cholmcc could you please change the title to say ProcessConfigurable ?

I'm not sure it only applies to ProcessConfigurable only - but will change to something similar.

I am not against this PR but I don't understand why you, Christian, use instead of normal configurables the ones of process switches?

I don't. i use one ProcessConfigurable in O2Physics:PWGMM/Rivet/Tasks/rivet.cxx called hepmc-no-aux and that is it. This is defined not via the relevant pre-processor macro exactly to allow a name scoping to the "family" of "hepmc" (i.e., "hepmc-no-aux" instead of "hepmcNoAux" in line with f.ex. "hepmc-recenter").

Yours,

Christian

@cholmcc cholmcc changed the title Update runDataProcessing.cxx to allow for - and _ in Configurabl…e<T> names Update runDataProcessing.cxx to allow for - and _ in (Process?)Configurable<bool> names Feb 28, 2024
@cholmcc
Copy link
Contributor Author

cholmcc commented Feb 28, 2024

BTW, if the fundamental requirement is that option name can be put into a JSON key, then perhaps the regular expression for the option should capture that requirement - perhaps

([^"']+)

That is, anything not a quote or double quote, since JSON keys can be any UTF-8 character string - including white-space and so on. One could even allow for '" if they are properly escaped.

Just a though.

Yours,
Christian

@aalkin
Copy link
Member

aalkin commented Feb 28, 2024

The code in question is also called for regular switches as far as I can tell.

@cholmcc absolutely not, this is specifically made to extract configuration parameter name from the string metadata attached to inputs, that is placed there based on process switches only. You can look at O2Physics codebase and find plenty of configurables with dashes and underscores.

Copy link
Contributor

This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days.

@github-actions github-actions bot added the stale label Mar 31, 2024
@github-actions github-actions bot closed this Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants