Skip to content

Conversation

@ktf
Copy link
Member

@ktf ktf commented Jan 30, 2026

  • Out of line and avoid usage of stringstream.
  • Remove non-sense abbreviations

- Out of line and avoid usage of stringstream.
- Remove non-sense abbreviations
@ktf ktf requested a review from a team as a code owner January 30, 2026 11:19
@github-actions
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-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1
async-2024-PbPb-apass2
async-2023-PbPb-apass5

@ktf
Copy link
Member Author

ktf commented Jan 30, 2026

Apart from some detectors and ALICE3 I most notably missed "MC" as an abbreviation. I suggest to add those in a separate PR. In any case, it should behave as before in the worst case scenario.

@mhemmer-cern
Copy link
Contributor

Hello @ktf
I am happy to see EMCal/EMC in there. If there would be a possibility to add more abbreviations, from the PWGEM Photon side we would like to add PCM (photon conversion method) and PHOS (for the converted data and the early Run3 data where PHOS was included).
Thanks a lot for looking into this!

@ktf
Copy link
Member Author

ktf commented Jan 30, 2026

@mhemmer-cern yes, of course. As I said, please let's see this PR green and merge it and then we can add everything which is missing.

@vkucera
Copy link
Collaborator

vkucera commented Jan 30, 2026

How many wagons will break because of the changed device names?

@ktf ktf merged commit 02a0aeb into AliceO2Group:dev Jan 31, 2026
12 checks passed
@ktf
Copy link
Member Author

ktf commented Jan 31, 2026

Tested on hyperloop. Merging.

@mhemmer-cern
Copy link
Contributor

I just did a quick test with one task that I have that is affected by this change.
I downloaded the config json, then updated the wagon to get the new task name and then just changed it in the downloaded file and uploaded it again. This way I was able to keep my configs.
An announcement with actions to take to ensure smooth transition should be send out to everyone in my opinion @ktf. Otherwise there could be a big confusion happening at the start next week.
As far as I could see the TPC and TOF PID service wagons are not affected since they already had the task names like that.

@ktf
Copy link
Member Author

ktf commented Feb 1, 2026

You mean that you have something which was -e-m-c and that becomes -emc?

@mhemmer-cern
Copy link
Contributor

You mean that you have something which was -e-m-c and that becomes -emc?

exactly, it was task-pi0-flow-e-m-c and is now task-pi0-flow-emc.

@vkucera
Copy link
Collaborator

vkucera commented Feb 1, 2026

I have found 36 affected workflows. I suspect that the number of affected wagons is much larger.

@ktf
Copy link
Member Author

ktf commented Feb 1, 2026

I can remove "-emc" and "-emcal" from the list, if you prefer, and then we allow people to migrate with proper instructions and so on.

@mhemmer-cern
Copy link
Contributor

I can remove "-emc" and "-emcal" from the list, if you prefer, and then we allow people to migrate with proper instructions and so on.

For me personally it is fine. I knew this change was coming and I was able to migrate properly. I just think that we should announce this change in the proper channel to make sure everyone affected is prepared.

@ddobrigk
Copy link
Contributor

ddobrigk commented Feb 1, 2026

Hi everyone, indeed, this is a little dangerous. On the positive side, last time I had something like this, the task was broken but switching to the new executable (straight from the old executable) made Hyperloop retain all configurable values - but if one switches to the wrong executables, there might be troubles as the sync might destroy the existing configurations. I agree it would be better to announce this maybe tomorrow morning, just in case. Thanks!

@vkucera
Copy link
Collaborator

vkucera commented Feb 1, 2026

Hi everyone, indeed, this is a little dangerous. On the positive side, last time I had something like this, the task was broken but switching to the new executable (straight from the old executable) made Hyperloop retain all configurable values - but if one switches to the wrong executables, there might be troubles as the sync might destroy the existing configurations. I agree it would be better to announce this maybe tomorrow morning, just in case. Thanks!

I don't think it can work the same with devices. There is no way that Hyperloop can be told to port configuration of one JSON block into another one.

@ddobrigk
Copy link
Contributor

ddobrigk commented Feb 1, 2026

Hi everyone, indeed, this is a little dangerous. On the positive side, last time I had something like this, the task was broken but switching to the new executable (straight from the old executable) made Hyperloop retain all configurable values - but if one switches to the wrong executables, there might be troubles as the sync might destroy the existing configurations. I agree it would be better to announce this maybe tomorrow morning, just in case. Thanks!

I don't think it can work the same with devices. There is no way that Hyperloop can be told to port configuration of one JSON block into another one.

You may be right, indeed: in my case it was only the executable name that changed ...

@ktf
Copy link
Member Author

ktf commented Feb 1, 2026

Let's discuss this tomorrow. I can either rollback the exceptions which break (which one?) or we can add an exception to the exceptions if applicable (e.g., adding {"e-m-c", "-e-m-c"} would have solved the incompatibility as well.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Feb 2, 2026

The main problem here is that even if everyone fixes their configs, including all service tasks, which is quite some work, we cannot run ever on an old tag again. We have to find a different solution.

@ktf
Copy link
Member Author

ktf commented Feb 2, 2026

Why do we need to fix every config? I assume most of the tasks (and in particular the service tasks) are either already compliant (I was surprised to learned we really had a task called something-e-m-c), or they have an override in any case (which again, this code does not change).

Regarding a general solution for this to be supported, I guess the easiest is just to ignore - when doing a lookup for the task name.

@jgrosseo
Copy link
Collaborator

jgrosseo commented Feb 2, 2026

At present the config key is task name and then configurable name. Of course everything can be changed but it is something we have to plan carefully and discuss. Let's grab a coffee one of these days.

@ktf
Copy link
Member Author

ktf commented Feb 2, 2026

Sounds good. As discussed privately I reverted everything apart from the white sheep. ;-)

For the record, the list of affected tasks is:

alice3-decayer-q-a
antimatter-absorption-h-m-p-i-d
cent-q-a
compatible-b-cs
d-edx-vs-occupancy-with-track-q-ainfo-task
efficiency-q-a
emcal-q-c
emc-event-selection-q-a
emc-vertex-selection-q-a
flatenicty-f-v0
lf-i-t-s-t-p-c-matching-secondary-tracks-q-a
net-proton-cumulants_-table_-q-a
nuclei-q-c
o-t-f-v0-qa
phos-cell-q-a
phos-clu-q-a
phos-trig-q-a
pid-t-o-f-generic
q-a-histograms
q-a-process-cent
q-cspectra-t-p-c
qcspectra-tpc
robust-fluctuation-bunch-crossing-q-a
run2-b-c-infos-converter
sg-exclusive-phi-i-t-sselections
strangeness-tracking-q-a-task
strangeness-tracking-q-c
strderived-gen-q-a
task-pi0-flow-e-m-c
track-q-a-converter
track-q-a-converter002
track-q-a-converter003
tree-writer-t-p-c-t-o-f
upc-event-i-t-s-r-o-fcounter
v0cascades-q-a
vertex-q-a

@mhemmer-cern notice that

  • emcal-q-c
  • emc-event-selection-q-a
  • emc-vertex-selection-q-a

will unfortunately go back to their original name.

@vkucera
Copy link
Collaborator

vkucera commented Feb 3, 2026

Dear all (adding @dsekihat),

TL;DR: This PR adds several problems while attempting to solve a problem which actually is not a problem.

Short explanation

The function used to correctly execute an unambiguous conversion from UpperCamelCase to kebab-case:

  • UpperCamelCase in -> kebab-case out
  • garbage in -> garbage out

This PR changes this behaviour to:

  • UpperCamelCase in -> kebab-case out
  • selected garbage in -> kebab-case out
  • other garbage in -> garbage out

Before this PR, there was a one-to-one mapping between camelCase and kebab-case names, which allowed for all related conversions to be simple, consistent, unambiguous, invertible. This encouraged readable proper camelCase names and made the struct-device-workflow-file correspondence rules trivial.
This PR makes this all fall apart.

Long explanation

  • The one-to-one mapping becomes many-to-one mapping, where many = 2^N, with N being the number of exception acronyms in the string. (Other acronyms still produce garbage.)
  • Out of these 2^N variations, only two have consistent capitalisation, but, according to Giulio, they should all be considered valid.
  • As a result, conversions from camelCase become inconsistent and ambiguous.
  • The inverse kebab-case -> camelCase conversion and the lowerCamelCase -> UpperCamelCase conversions become impossible because of the ambiguity.
  • Unambiguous case-sensitive full-text searches are no longer possible and have to be replaced with ambiguous case-insensitive searches with false positives.
  • The struct-device-workflow-file correspondence rules can no longer guarantee consistency which has been their sole purpose.
  • Unreadable names containing multiple adjacent acronyms in all capitals are now supposed to be considered valid variations.
  • Same acronyms are now allowed to have different capitalisations in different strings which encourages more inconsistency.

Overall, this throws consistency and readability out of the window, which completely undermines the two main purposes of naming conventions in general.

The very idea that camelCase with fully capitalised acronyms is a viable naming convention (which is the premise of this PR) is known to be flawed.
It is a known problem with a known solution: CamelCase capitalises only the first letter of each token. (See a detailed breakdown here and its references. https://gist.github.com/adashrod/66564c690906c9b774e77ddacbd06e1b)
All naming styles destroy the capitalisation of the input strings by construction. It is the intended behaviour and there is nothing broken about it. There is no good reason to make exceptions for camelCase, especially since it relies on using capitalisation as a word divider, so distorting the capitalisation with exceptions completely messes up the whole concept of converting prose into code names.

Please let the conversion function do its job, as it used to do.

@ktf
Copy link
Member Author

ktf commented Feb 3, 2026

This PR changes this behaviour to:

  • UpperCamelCase in -> kebab-case out
  • selected garbage in -> kebab-case out
  • other garbage in -> garbage out

What you call "selected garbage in" is the name of the ALICE subdetectors or common HEP acronyms.

Citing our official coding conventions:

All names in C++ code follow camel case convention. This is
the practice of writing compound words so that each word
begins with a capital letter. No underscores are allowed.
Acronyms are the only exception : they can be capitalized.

Please disable this specific linter test as it conflicts with a choice which we made more than ten years ago and that people have been happy with for the last ten years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants