[QC 1123]: using hash when description is too long #2171
Conversation
|
There are some tests failing on macOS. It's not clear to me why only there... |
|
|
||
| #include <Headers/DataHeader.h> | ||
|
|
||
| namespace o2::common::hash |
There was a problem hiding this comment.
| namespace o2::common::hash | |
| namespace o2::quality_control::core |
Since the header is anyway a part of the QualityControl framework library and the intended use is within QC, I would not put it in the general O2 namespace.
| namespace o2::common::hash | ||
| { | ||
|
|
||
| o2::header::DataDescription createDataDescription(const std::string& name, size_t hash_length = 4); |
There was a problem hiding this comment.
| o2::header::DataDescription createDataDescription(const std::string& name, size_t hash_length = 4); | |
| o2::header::DataDescription createDataDescription(const std::string& name, size_t hashLength = 4); |
| @@ -109,7 +109,7 @@ class AggregatorRunner : public framework::Task | |||
| static framework::DataProcessorLabel getLabel() { return { "qc-aggregator" }; } | |||
| static std::string createAggregatorRunnerIdString() { return "qc-aggregator"; }; | |||
| static std::string createAggregatorRunnerName(); | |||
| static header::DataDescription createAggregatorRunnerDataDescription(const std::string& aggregatorName); | |||
| static header::DataDescription createAggregatorRunnerDataDescription(const std::string& aggregatorName, size_t hashSize = 4); | |||
There was a problem hiding this comment.
| static header::DataDescription createAggregatorRunnerDataDescription(const std::string& aggregatorName, size_t hashSize = 4); | |
| static header::DataDescription createAggregatorRunnerDataDescription(const std::string& aggregatorName); |
This could be discussed, but I think it would be safer to assume always the same hash length for a given QC actor type. Otherwise, someone who wants to subscribe to any QC Data Processor would have to check the InfrastructureGenerator implementation (instead of just relying on information in a header) to use the correct hash length and would be exposed to incompatibilities if we change it.
I would propose that hashLength is a constant inside each of such create*DataDescription methods.
There was a problem hiding this comment.
sure, I will change it for all of the factory methods. I just saw there is written N for hash length in ticket, so I kinda propagated it everywhere...
There was a problem hiding this comment.
to be clear, the general function in HashDataDescription should still have the argument, the component-specific ones should use constants.
| } else { | ||
|
|
||
| std::stringstream ss{}; | ||
| ss << std::hex << std::hash<std::string>{}(name); |
There was a problem hiding this comment.
Perhaps mac failures are related to the implementation of std::hash? I think it would be good to have the hash to return the same result on any platform and any execution, because we might generate a workflow template on one OS and use it in a different one.
There was a problem hiding this comment.
yes, it does... I thought that it might happen, but I still wanted to try it
There was a problem hiding this comment.
I had an implementation of djb2 hash, but I took it away when I saw that std::hash is used for timers.
There was a problem hiding this comment.
then indeed it's weird that it does not fail for timers...
There was a problem hiding this comment.
https://github.com/AliceO2Group/QualityControl/blob/master/Framework/src/TaskRunner.cxx#L307-L320
I guess that there is not a test for exact task name of timers
Framework/src/Check.cxx
Outdated
| framework::OutputSpec Check::createOutputSpec(const std::string& checkName) | ||
| framework::OutputSpec Check::createOutputSpec(const std::string& detector, const std::string& checkName) | ||
| { | ||
| return { "QC", createCheckDataDescription(checkName), 0, framework::Lifetime::Sporadic }; | ||
| using Origin = o2::header::DataOrigin; | ||
| Origin header; | ||
| header.runtimeInit(std::string{ "C" }.append(detector.substr(0, Origin::size)).c_str()); | ||
| return { header, createCheckDataDescription(checkName), 0, framework::Lifetime::Sporadic }; |
There was a problem hiding this comment.
These changes duplicate #2170 . Could you please remove them from here and address the comment in the other PR?
There was a problem hiding this comment.
sure, sorry. I based this branch by mistake on the branch for that task.
No description provided.