-
Notifications
You must be signed in to change notification settings - Fork 21
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
use (but don't require) denv in ldmx env #1269
Conversation
Right, I've tried to look at this as if I was a completely new user
. scripts/ldmx-denv-init.sh Gives
Same on bash, so that's not the issue The reason for this is of course that i haven't run denv init yet. I would appreciate if the new scripts would check
It looks like this is what Very minor: |
Thank you for giving this a try Einar!! 🥇 I have employed shellcheck to start helping debug these scripts and I've already found many patches to make. One of the issues shellcheck found seems to be the primary reason the
Unfortunately, finding the path to the current source file in a POSIX-sh compliant way is hard (or impossible when sourcing a file rather than executing it?) so I'm investigating solutions. We might need to abandon a POSIX init script in favor of documentation if there isn't another way to deduce |
I did the following on UCSB's cluster which uses
leads to
Then I run
which leads to
(now same thing happens with The singularity version is 3.5.2, so I guess this not working out of the box is expected. |
🙌 thank you @tvami 🙌 There are two separate "issues" that your comment brings up. singularity versionUCSB also has
to prefer the newer version. The last error you see after init typoThe first error you see from running
is because there is a typo in the README I wrote (now fixed). It should be
In this call, we are using
|
OK this worked now for the environment, however,
Doing
then leads to
One minor technical comment I'll have is: now this adds the On the philosophical side, why not get rid off the |
While I appreciate the idea of a cleanup, and it makes sense to be able to share useful functionality, I feel a little nervous about making the core LDMX software dependent on a package which is owned/maintained by a specific individual. Does this make sense in a long-term manner? |
@tvami I think both of your errors make sense in the context that the As far as the location of the I agree with you that supporting both @jmmans I think your concern is well founded. I've been trying to think about the long term sustainability of this project especially if LDMX adopts it. I actually presented this project to the Hep Software Foundation last September where I received good feedback but I was unsure on the next steps. Perhaps I reopen discussions with them about transitioning ownership of denv to HSF? Another obvious option is for me to transfer ownership to LDMX-Software or have LDMX-Software keep a fork. |
In order to get running functional, we (currently) need to edit the After the first attempt to run
We then are free to update them as we see fit since they are only copied into place if they don't exist. I wrote the following into the
This extra shell code updates the necessary environment variables in the container so that
The
|
I'm going to delay this until after May 1st. It will be a good example for how to update a branch (and what the resulting commit history will look like) and I don't have time to polish this as much as I think is necessary for it to be ready to merge. |
698cafa
to
96526b4
Compare
This last force push was me following the online instructions for updating a branch from v3.4.0 to v4.0.0. |
remove impossible POSIX-sh setup script and instead encourage users to wrap denv specifically for their shell or use denv directly
avoid code repetition by calling compile from within recompile and fire
The more I've thought about it the more I'm displeased with this solution.1 Since I think the long term solution is to pick a task runner to handle these tasks for us. While we wouldn't have control over how this task runner is implemented, it would simplify defining new tasks and allow us to have a single (and short!) file that lists all of the common commands and how they are run. For this reason, I'm going to close this PR and focus more on writing a just file for developing ldmx-sw. In addition, there is an open issue with denv (tomeichlersmith/denv#80) that I want to resolve before fully adopting denv within LDMX. Footnotes
|
I am updating ldmx-sw, here are the details.
What are the issues that this addresses?
This resolves #1232 directly, resolves #1248 directly, and resolves #1252 indirectly. I do this by moving the complicated container interaction scripting into its own project such that it can be more directly tested and version controlled. The environment scripts here are then just using this other shell program to help replicate the current environment.
The long-term benefit of isolating the container interaction into its own program amounts to allowing users to do the same process we do with ldmx-sw with any of their software. Below I have two examples that I do regularly and one idea that I could see becoming commonplace if we adopt
denv
within LDMX.Pin ldmx-sw Version for Production
I can use
denv
to choose an ldmx-sw version and use it pre-compiled. It is pinned and is not cluttered by my developments happening elsewhere on my system.Pin Python and Packages
I often want to develop a python analysis to share with someone else and it is very helpful to be using the same python version and the same version of all the packages.
pip
andvenv
offer a good way to pin the python package versions, but often times it is difficult to align the python version across systems. Luckily for us, python builds container images that we can use directly with denv.Sharing Environments
In some cases, the workspace that I use with
denv
(the directory where I randenv init
) is also agit
repository. In these cases, I can commit the config written by denv so that others who want to replicate the work I did with that code can simplyRequested Feedback
Note that I am not removing the current
ldmx-env.sh
script. It has many peculiarities that we have gotten used to and I have not replicated all of them withdenv
. This was intentional since many of these peculiarities are born out of just trying to get it to work and I have now figured out a better solution. This is a first PR to make usingdenv
an available path for people looking to develop ldmx-sw. I wish to have reviews of this PR focus on the followingldmx-env.sh
to usedenv
for container interaction #1232 but I want to re-open the discussion with this more concrete proposal. I also want to point out that I am not committed to the draft roadmap I wrote in that issue, it was just me dumping my thoughts onto GitHub.ldmx
here as shorthand) to more simply wrapping another program (strategy calleddenv
here as shorthand). Even if we stay with a bash environment script with bash functions wrappingdenv
indefinitely, there are still some aspects of the workflow that will change.To Do
/cvmfs/unpacked.cern.ch
in environment script #1252 (comment)ldmx-denv-transition.sh
)config
,use
,pull
,mount
,setenv
Change default installation fromTaken off list because we need to updateldmx-sw/install
to$LDMX_BASE/.local
. This aligns withdenv
's design choice to make the workspace (for us,LDMX_BASE
) the home directory in the container which means the paths under~/.local
are often already included by the variousPATH
variables.PYTHONPATH
no matter what and I like how easy it is for folks to delete the build and the install when trying to debug.PATH
variables in the${LDMX_BASE}/.profile
file. A template file is written bydenv
if it doesn't already exist. Copy environment setup to/etc/skel/.profile
docker#94 We need to do this even if we changed ldmx-sw's default installation to${LDMX_BASE}/.local
because we do not put our python config modules into a directory that is automatically searched by python (unless it is included inPYTHONPATH
).Context
To provide further context. The
ldmx-denv-init.sh
file just initilizes denv trying to be POSIX-sh compliant so that it could be used in other POSIX shells. Theldmx-denv-transition.sh
file is a bash-specific environment file that wraps denv with various bash functions to try to mimic the behavior of the currentldmx
command. Below, I have these translations written out in a more human readable form.ldmx
Commanddenv
Equivalenthelp
help
config
config print
use
config image <image>
pull
config image pull
mount
config mounts <dir>
setenv
config env copy
run
checkout
source
list
clean
Explanations for Removals
All of the
ldmx
commands that do not have adenv
equivalent are not included withindenv
because they do not directly pertain to interfacing with the container runner. I also am proposing removing them entirely from our ecosystem largely because they are either (a) broken, (b) not used widely, or (c) both.run
: In the original bash functions, this avoids the directory movement outside of the container and directly passes the arguments to the entrypoint script inside the container. I am removing this because it is overly complicated to replicate withdenv
in a robust way especially since the easiest way to replicate it is to tell users to justcd <dir>; denv <cmd>
.checkout
: this tries to code a more efficient combination ofgit checkout
andgit submodule update
so that users can more easily switch ldmx-sw branches; however, this is not functional. Moreover,git
provides a better description of how to switch between branches with different submodule commits and I think we should simply guide developers to this resource.source
: this is here to allow users to run multipleldmx
commands at once intending to have a single file defining the configuration of the environment conducted byldmx
. This command is not necessary indenv
sincedenv
stores the environment configuration within a text file at<workspace>/.denv/config
(for us,ldmx-sw/../.denv/config
since the workspace is the parent of ldmx-sw).list
: I tried to write some plain-shell code to list the available tags of the image. Since time of writing, DockerHub has changed its API breaking this code. Nobody else has noticed (or even if they have noticed, it wasn't a big enough deal to make an issue or post on slack), so I think removing it and just pointing people to the GitHub releases and DockerHub is good.clean
: This is the messiest command and has three sub-tasks.src
: Clean out all untracked files from the source file tree. Just a wrapper aroundgit clean
basically and with the moving of the generated files out fromFramework
and into thebuild
directory, something more complicated thatrm -r build install
is not necessary anymore.container
: Remove the downloaded container images. This is a dangerous action and I feel more comfortable instructing people on how to list the images already downloaded and remove them manually (either withdocker image rm
orrm
depending on the runner).env
: reset the environment variables.denv
does not store any important information in environment variables preferring instead to write and read the config file.