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

Add scripts to compile, and recompile and fire a config (intro of ldmx compile and ldmx recompFire) #1282

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Apr 10, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1278

Check List

  • I successfully compiled ldmx-sw with my developments

  • I ran my developments and the following shows that they are successful.

The way I ran this was to copy the pn validation config to my location and named it config.py, then run

############################# Usage #############################
# ldmx ./scripts/ldmx-recompileAndFire.sh
# for changing the defaults
# ldmx setenv LDMX_COMPILE_CORES=<num cores to be used>, i.e.
# ldmx setenv LDMX_COMPILE_CORES=16
# or
# ldmx setenv LDMX_COMPILE_BUILD=<build location>

also ran

ldmx compile

and

ldmx recompFire config.py
  • I attached any sub-module related changes to this PR.

N/A

@tvami tvami self-assigned this Apr 10, 2024
Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

This review is mainly focused on the overall design choices. These scripts are functional and so I don't mean to block any merging - if I'm overruled by other devs, I'd be happy to merge this anyways. With that caveat, I am going to go through my general design-choice comments with their motivations in no particular order.

  1. Don't have these scripts be sourced: This could pollute the host environment (e.g. with the variables CORES and BUILD) which could confuse other programs. I would imagine these scripts as shell programs (i.e. with a shebang #!/bin/bash and executable permissions chmod +x) so that they are run in their own process.
  2. Have this run inside the container: Every time you call ldmx, you trigger a container open/close cycle. This is not a huge performance issue on most systems, but it can be helpful to remember that and avoid more open/closes by putting an entire script inside the container. This is simply removing the ldmx prefixes in the script and forcing the user to run the script with the ldmx prefix by checking for the existence of container-only files (/.dockerenv or /singularity).
  3. Avoid directory movement: cmake, when given full paths for -B, -S, and --build handles the movement for us, so we can drop the cd. This has the added benefit of users not ending up in a different directory if the kill the process with ctrl+C1.
  4. Pass script arguments to commands and have script-specific args be environment variables: I think this is a more natural way to partition the arguments supported by the script and the (larger) set of arguments supported by the command(s) being run in the script. Combined with (2), this would also naturally lead to ldmx setenv LDMX_COMPILE_LOCATION=/full/path/to/ldmx-sw; ldmx ./scripts/ldmx-compile.sh or something. Moving the script-specific args to environment variables also supports passing the entire command line to fire which allows users to have a config script that parses the command line to tune its configuration (something I use quite a lot especially to define input files).
  5. shellcheck: https://www.shellcheck.net/ Install it or use the browser version. It just helps so much with catching tricky shell bugs and helps teach better shell coding practices. I have a .shellcheckrc that I think would help tune shellcheck for our scripts which gives you the options I would use and the warnings/errors I would disable.

Footnotes

  1. Probably? The ldmx suite of bash functions does do some directory movement and so I've noticed that I sometimes end up in LDMX_BASE after ctrl+C if I'm on a host running singularity. This aesthetic confusion would be resolved by using a program to launch the container (e.g. denv) which puts any directory movement into its own process rather than the current one.

@bryngemark
Copy link
Contributor

to add to @tomeichlersmith's ideas, i was thinking, wouldn't it be neat if the syntax to run these helper scripts were just ldmx recompile and similar? i.e. making them a function like many of the others we already have in ldmx-env.sh?

@tvami tvami marked this pull request as draft April 11, 2024 21:36
@tvami tvami changed the title Add scripts to compile, and recompile and fire a config Add scripts to compile, and recompile and fire a config (intro of ldmx compile and ldmx recompFire) Apr 11, 2024
@tvami
Copy link
Member Author

tvami commented Apr 11, 2024

Thanks for the suggestions! I made it work on my Mac+docker, now I'm going to test it out on a linux too and if it works, I can change it back from draft

@tvami tvami marked this pull request as ready for review April 11, 2024 23:03
@tvami
Copy link
Member Author

tvami commented Apr 11, 2024

Works nicely on UCSB's cluster too, which uses apptainer (and is a linux)

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

The only functionality change I have is about passing arguments so that these scripts work with configs that need additional command line arguments. I have other comments that I'm okay with dropping if folks don't like them.

scripts/ldmx-compile.sh Show resolved Hide resolved
scripts/ldmx-compile.sh Outdated Show resolved Hide resolved
scripts/ldmx-env.sh Outdated Show resolved Hide resolved
scripts/ldmx-recompileAndFire.sh Outdated Show resolved Hide resolved
scripts/ldmx-recompileAndFire.sh Outdated Show resolved Hide resolved
@tvami
Copy link
Member Author

tvami commented Apr 12, 2024

I tested with a config that takes the number of events from an argument, works nicely!

ldmx recompFire yetAnotherConf.py 100

@tvami tvami merged commit 935fe31 into trunk Apr 12, 2024
1 check passed
@tvami tvami deleted the iss1278 branch April 12, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify (re)compilation and use a script instead
3 participants