pcdshub / engineering_tools

A repository of scripts, configuration useful for the PCDS team
Other
4 stars 26 forks source link

use daqutils for daqmgr hutches #203

Closed monarin closed 1 month ago

monarin commented 1 month ago

Description

ZLLentz commented 1 month ago

I'm going to reformat your description here to restore the "Description" header, approve the workflow run, and add a few people as reviewers

ZLLentz commented 1 month ago

For reviews, I'm pinging Silke because she has most of the commits on the previous restartdaq script, but she may not have time, so I'm also pinging Vincent because he has the most commits on this repo out of all the CDSOs.

monarin commented 1 month ago

HI all, sorry for my delayed response. I really appreciate a lot of comments and feedbacks here! Sorry if I can't reply directly to all these suggestions now, I hope to be sitting down and working with both chris and silke next week so we can work through these. Hopefully the next version will be more ready.

ZLLentz commented 1 month ago

I should have mentioned this last week, but the core intent and implementation here (the main points in the PR description, such as switching to slurm) were good and didn't need any scrutiny. Aside from some questions for understanding, my comments are largely targeting maintainability (putting all the cli parsing in one place, avoiding confusing error tracebacks, etc.).

I hope I didn't come across as too critical and I'm sorry if I did.

silkenelson commented 1 month ago

For the old setup, I wanted to cd into the same directory so that the temporary files such as pid.txt would always end up in the same space.

ZLLentz commented 1 month ago

I think this is good to merge if someone tries it out and confirms it does what they need

monarin commented 1 month ago

We decided to rename the slurm-for-daq manager from daqbatch to daqmgr. This is to avoid technical details in the name. We needed to complete this name change while it's still early enough. Unfortunately, this happened to be in the middle of this PR and I had to complete the work here too to verify the test at tmo and rix.

monarin commented 1 month ago

I'm looking at the pre-commit errors. Can anyone advise how to run pre-commit test interactively?

ZLLentz commented 1 month ago

First, please ignore any pre-commit warnings from parts you did not edit. We're in the process of cleaning some things up in #204.

Some of the pre-commit jobs will fix the formatting for you, others will just complain.

To run pre-commit interactively:

source pcds_conda  # /cds/group/pcds/pyps/conda/pcds_conda if not on PATH
pre-commit install  # optional, makes pre-commit run before every commit for this repo
pre-commit run --files <filename>  # make pre-commit run on a specific file
ZLLentz commented 1 month ago

(for example, please ignore shellcheck for now- it's not on you to edit the entire restartdaq script)