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

allow for run-type specific run ranges #98

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

corrodis
Copy link
Collaborator

I propose we handle the actual run type-specific run ranges in the database with different run sequences and a trigger function. However, this modification in this pull request is needed to read back the right run number after inserting a new run_configuration record that generates the run number.

@antoniogioiosa
Copy link
Contributor

My understanding is that you need to change the table run_condition by adding a run_type column.
Changes on the run_condition table need to involve Ray in the review task, I think.

@rlcee
Copy link

rlcee commented May 17, 2024

My first question is what do you mean by this?

run type-specific run ranges

The plan we are working towards, and should be using when this code is active, is that all runs, from all types, come from one sequence. The assignment of a run range to tracker VST, for example, was a temporary procedure that I would not expect to be reflected in this code. I have not been involved in discussions of how to transition between the two, and maybe some temporary code might be needed there.

@corrodis
Copy link
Collaborator Author

Thanks a lot. I'm also including @pavel1murat and Ralf here.

If I understand the current implementation correctly, the run_condition (set at the configuration state) is run_type independent. Each run number (unique key in run_configuration) has a run_type. That run_type, that we currently set through OTSDAQ_RUNINFO_DATABASE_RUNTYPE, is what I propose to use to determine the right run-range.

We are currently working on tracker and CRV vst, and I think we would like to use the latest software and database schema for these tests. From my point of view, I think the code we use today needs to reflect these special run ranges IF we want to use them. My proposal follows the documentation here (https://mu2ewiki.fnal.gov/wiki/RunNumbers#Reserved_run_numbers). In practice, this means that if the run_type is TRK VST (1), we get run numbers in the range 100000-100999, if the run_type is CRV VST (4), we get run numbers in the range 103000 to 103999, in all other cases, we get run numbers in the range 105000 to 999999. The implementation of the run-ranges is in the database, not the code base, so we can change it easily. This mapping is what I mean with "run type-specific run ranges".

If we want to decouple our ongoing work today from the final running conditions, I think we'd need to make a clone of the production schema to be used for commissioning work. In either case, I think the proposed changes in this pull request are desired to have in. It allows the scheme I described above and is fully backward compatible to a scheme where we only have one run range in the database.

Simon

On Fri, May 17, 2024 at 7:29 AM Ray Culbertson [email protected] wrote:
My first question is what do you mean by this?

run type-specific run ranges

The plan we are working towards, and should be using when this code is active, is that all runs, from all types, come from one sequence. The assignment of a run range to tracker VST, for example, was a temporary procedure that I would not expect to be reflected in this code. I have not been involved in discussions of how to transition between the two, and maybe some temporary code might be needed there.


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were assigned.

@rlcee
Copy link

rlcee commented May 17, 2024

If we were to continue the assigned run ranges while making inserts in run_configuration, then there would need to be several changes. I expected that as soon as you were getting your run number from run_configuration serial, you would get it from the one serial source. So immediately crv and trk would be sharing the same sequence, starting at 105000. Do you, and others, feel this won't work?

Also, a technical aside. In the insert, you can add a phase "returning run_number" as a secure way to get the result of the serial trigger, instead of reading back the last entry, which has some tiny chance of being wrong.

@corrodis
Copy link
Collaborator Author

I don't have any strong opinion about separate run ranges and don't see an issue with everyone getting sequential numbers. I was under the impression that run ranges were a desired feature that we wanted to accommodate for in our VSTs.

If we go for different run ranges, my proposal was to use different sequencers and a trigger function to pick the right one. I think that would work quite well? Everything would be handled in the database.

I fully agree that getting the run_number as return value is much more elegant and avoids the corner case where two systems start at basically the same time. I will change this pull request accordingly. This also decouples the discussion of this pull request from the run range discussion. Thanks a lot!

…ING for run_number and configuration_id creation
@corrodis
Copy link
Collaborator Author

Copy link
Contributor

@rrivera747 rrivera747 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@normanajn
Copy link

@glenn-horton-smith @antoniogioiosa Please review

@eflumerf eflumerf added the enhancement New feature or request label Nov 4, 2024
Copy link
Contributor

@antoniogioiosa antoniogioiosa left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@rlcee
Copy link

rlcee commented Nov 5, 2024

Is this still run-range based or is it now purely sequential? Thanks

@antoniogioiosa
Copy link
Contributor

Hi Ray, we decided to use only the sequential method during our last discussion.

@glenn-horton-smith
Copy link
Contributor

Looks good to me too.

@@ -260,8 +260,8 @@ unsigned int DBRunInfo::claimNextRunNumber(unsigned int conditionID, const std::

snprintf(buffer,
sizeof(buffer),
"select max(run_number) from %s.run_configuration;",
dbSchema_);
"select max(run_number) from %s.run_configuration WHERE run_type = '%s';",
Copy link
Contributor

Choose a reason for hiding this comment

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

@corrodis after the last TDAQ Ops meeting, do you agree we should revert this line? And then merge the rest of your fixes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that sounds right to me. I think the "RETURNING" is the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants