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

Thread pinning #243

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Thread pinning #243

merged 8 commits into from
Feb 21, 2024

Conversation

plasorak
Copy link
Contributor

Better thread pinning:

  • Correct a bug that made thread-pinning throw an error in some situations
  • Remove the spurious error logs that appear when one tries to pin the threads without a thread-pinning script setup in boot.json.
  • Reduce the logging of the thread pinning. Now the full glory of the thread-pinning script execution is only printed when nanorc was started with --loglevel debug.
  • The philosophy of the 2 points above is that one only gets a readable message that the thread-pinning script is executing, and, if the thread-pinning execution fails, nanorc will print the error.
  • Generalise the thread pinning, users can now specify after which transition which thread pinning is executed.

@plasorak plasorak changed the base branch from develop to prep-release/fddaq-v4.3.0 February 14, 2024 12:47
@plasorak plasorak added the enhancement New feature or request label Feb 14, 2024
@bieryAtFnal bieryAtFnal added the miscellaneous deliverable A change that is/will be part of a release but is not substantial enough to be a daq-deliverable label Feb 14, 2024
@plasorak
Copy link
Contributor Author

This needs to be used with DUNE-DAQ/fddaqconf#20 (but not necessarily, as this is backwards compatible)

@wesketchum
Copy link
Contributor

Just tested at EHN1. Works / is backwards compatible with existing configs for workflow at NP04. Reduced messages are very nice, though could additionally suppress the details on the pin command. Fixes the spurious error on pin_threads for WIBs/hermes/etc.

I would be happy to see this in fddaq-v4.3.0, even without DUNE-DAQ/fddaqconf#20.

@@ -65,14 +65,13 @@ def ls(obj, legend):


@click.command()
@click.option('--pin-thread-file', type=click.Path(exists=True, resolve_path=True), default=None)
@click.argument('pin-thread-file', type=click.Path(exists=True, resolve_path=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this change is not backwards compatible, right? It means that a pin_threads command now must have an argument of the thread pinning file, and won't default to what has been set in the environment. This may be ok, but would break instructions/workflows that have been given to others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, but if one has more than one thread pinning file, I'm not sure how I should choose it without specifying an argument here.

@wesketchum
Copy link
Contributor

@plasorak is it possible to split out the changes on the verbosity of print messages and the errors on WIB/hermes/etc. blocks versus the other updates that are tied to the configuration? If so, that may be enough to get in now, and allow for further discussion if needed on the other elements, assuming that is needed.

@plasorak
Copy link
Contributor Author

I can certainly cherry-pick the commits which reduce the printout. For the WIB/Hermes errors, if you are referring to DUNE-DAQ/ehn1-operations-issues#19, yes that can be done too.

@plasorak
Copy link
Contributor Author

done here: #244

Copy link
Contributor

@wesketchum wesketchum left a comment

Choose a reason for hiding this comment

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

Tested at EHN1 and it seems to be working. Thanks!

@plasorak plasorak merged commit f38ec68 into prep-release/fddaq-v4.3.0 Feb 21, 2024
@plasorak plasorak deleted the plasorak/thread-pinning branch February 21, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request miscellaneous deliverable A change that is/will be part of a release but is not substantial enough to be a daq-deliverable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants