2021-01 cyrush workarounds and tweaks #46

cyrush · 2021-01-23T00:00:30Z

This branch includes:

A workaround to get back to point where the host config is generated in the prefix dir (place created host config in --prefix dir #44)
Addition of a -j option to control build jobs (override what is presented in configs for more control)
Addition of a json file option to control (spack spec is hard to read vs previous non verbose output #45)

WIP.
I don't expect us to merge as is. Need a better solution for #44, we should discuss.

Note: I am using this branch to allow me to move forward with updating uberenv for conduit.

adrienbernede · 2021-01-27T22:59:10Z

uberenv.py

@@ -814,9 +842,15 @@ def install(self):
                    print("[install complete!]")
        # otherwise we are in the "only dependencies" case and the host-config
        # file has to be copied from the do-be-deleted spack-build dir.
+        # -------
+        # NOTE: USING DEV BUILD, there is no `spack-build` dir


@cyrush yes there is a spack-build dir, it is created as the source top dir.
The role of dev-build is to use the local version of the sources, that's why it is convenient: the host-config gets generated next to the local version of the source (in src/spack-build).
Then, uberenv moves the host-config file from src/spack-build to src, and removes src/spack-build.
If I understand correctly, that's where is becomes a problem for you: you may have several host-config files there.

Correct. Overall, I want to avoid creating or deleting directories anywhere other than the prefix.

If things are limited to the prefix, I can avoid surprises.

I though I checked to see if the spack-build was created for my case, and I didn't see evidence it happened (maybe b/c it was deleted before I could check) but I thought I put in echos to see if ubrerenv saw it. I can double check when I am back working on this.

I removed this comment, not sure if its still an issue but it relates to a path I am not testing.

white238 · 2021-03-10T21:39:37Z

uberenv.py

        if not self.use_install:
            install_cmd = "spack/bin/spack "
            if self.opts["ignore_ssl_errors"]:
                install_cmd += "-k "
            if not self.opts["install"]:
-                install_cmd += "dev-build --quiet -d {0} ".format(self.pkg_src_dir)
+                if self.uberenv_package_name:
+                    # fake package path


This isnt always a fake package. This would also be helpful for installing an actual package that exists. Or am i missing something?

Like if i was in serac and wanted to build axom in their setup.

Though I'm not sure why this is toggling dev-build vs install.

if uberenv_package_name exists, it doesn't use the dev build logic. that pretty much it

I added self.use_dev_build as a better way of identifying the logic

white238 · 2021-03-10T21:46:41Z

uberenv.py

@@ -150,6 +150,11 @@ def parse_args():
                      dest="package_name",
                      default=None,
                      help="override the default package name")
+    # overrides uberenv_package_name
+    parser.add_option("--uberenv-package-name",


How is this different from --package-name that already exists?

its presence triggers non dev build for the "non install" case (like ancient ubrerenv)

--force-spack-install?

then you would end up with spack --package-name=fakepackage --force-spack-install or just spack --force-spack-install which would end up with default package for the project.

My concern is how is a user supposed to know the difference between:

spack --package-name=foo
spack --uberenv-package-name=foo

without digging into the code.

Better names are certainly welcome.

I think this will be set only in the json file for the most part.

use case is to have uberenv work out of the box to build tpls when the --install option is not passed.

Having the option only be in the json file might be another option?

Engage-Cyrus-Mode? =)

I don't fully understand the use case, but I agree with @white238 that this should either not be exposed to the command line or have a better name.

In either event, I think it needs to be better explained.

I am fine with removing this as an option from the command line.

kennyweiss

@cyrush -- I think I get the general idea, but I am not sure I understand what the exact problem is and how this solves it.

The logic here seems like it can get pretty hairy and difficult to maintain.

Is the problem that you want to control where the host-config gets generated?
If so, would it be simpler to have spack generate the host-config to its build/scratch directory
and allow uberenv to specify where we want to copy this file? (with a reasonable default)?

Or is there a different issue that I'm missing?

kennyweiss · 2021-03-11T01:47:42Z

uberenv.py

@@ -150,6 +150,11 @@ def parse_args():
                      dest="package_name",
                      default=None,
                      help="override the default package name")
+    # overrides uberenv_package_name
+    parser.add_option("--uberenv-package-name",


I don't fully understand the use case, but I agree with @white238 that this should either not be exposed to the command line or have a better name.

In either event, I think it needs to be better explained.

kennyweiss · 2021-03-11T01:48:10Z

uberenv.py

@@ -170,6 +175,12 @@ def parse_args():
                      default=pjoin(uberenv_script_dir(),"project.json"),
                      help="uberenv project settings json file")

+    # option to explicitly set the number of build jobs


kennyweiss · 2021-03-11T01:51:30Z

uberenv.py

        try:
            setting_value = self.project_opts[setting]
        except (KeyError):
-            print("ERROR: '{0}' must at least be defined in project.json".format(setting))
-            raise
+            if optional:


Am I reading this right?

It tries to get the setting from project_opts (which is read in from project.json), and if it is not there (KeyError)
then it throws an error when the setting is optional but returns None when it it not optional?

Should it instead be:

Suggested change

if optional:

if not optional:

Since this behavior is a bit complex, can you please add a docstring to explain what this is supposed to do?

Maybe the issue is about what optional is modifying.
Is it saying that:

having it in self.project_opts is optional

having it in self.opts is optional

having it in both of these is optional

my logic was wrong. (note this is still WIP)

I was surprised that many things are required in the json file, even though they may not be applicable. Like:

pacakge_final_phase

We have a check in the logic that applies it:

if self.pkg_final_phase: install_cmd += "-u {0} ".format(self.pkg_final_phase)

But you can't actually run uberenv unless it is there is a json file entry.

Thanks @cyrush -- I agree that we should have sensible defaults, when applicable, to reduce the necessary entries in the project.json file

kennyweiss · 2021-03-11T02:13:52Z

uberenv.py

                if self.pkg_final_phase:
                    install_cmd += "-u {0} ".format(self.pkg_final_phase)
            else:
                install_cmd += "install "
+                if self.opts["build_jobs"]:


It's a bit hard to see how this all lines up w/in github, but it seems that these two lines appear in both branches:

if self.opts["build_jobs"]: install_cmd += "-j {0}".format(self.opts["build_jobs"])

It so, and if the order of command line args don't matter, they should be pulled out one level

good catch, i'll refactor

cyrush · 2021-03-11T18:59:42Z

For context: I took several passes are using the dev build, including trying to inject fine grained control of the generated host config location, but I was not able to get parity with the original "uberenv fake" package approach.

The goal for the fake package is support is to retain the workflow driving the setup of all the TPLs required for development when uberenv is invoked with no args (relying on the project json file) and to also support deployment with install.

uberenv_package_name is the original json entry for uberenv, so I stuck with that.

The "uberenv fake" package helps in the following ways:

Semantics of the fake package are just like any other spack package. Having a successfully installed fake package that we can query is helpful, we can look at things in the .spack dir to solve mysteries. We can actually uninstall the fake package on or way to re-gen the host config, etc.
Following best practices for Spack proper, the default variants don't line up with what we need out of the box for development. With the fake package, we can make different choices (docs on, python on, etc) and support development out of the box. Consistent out of the box is very important -- we do development in ephemeral places (containers and scratch spaces) and have to bootstrap our dev env all the time.
The host config lives with the install in the prefix dir (and in the spack installed fake package dir). It is never written outside of the prefix or the spack stage area. This is really important for both development and deployment. I typically use between 2 and 4 uberenv prefix dirs per project (replicated across several projects). My CMake work flow is basically "use this ubrerenv prefix dir" via using the host config inside. If I delete or move the my uberenv dirs I don't have to hunt down files elsewhere. Self contained greatly simplifies things.

Draw backs of the fake package approach:

Inheritance isn't really inheritance in spack, you can tangle with some meta programming magic or confusing variant tweaking to get a good state.
Some times it downloads things needlessly.

Since I have used the fake package approach for many years, the downsides are a known quantity. As I upgrade I am interested in vcpkg support and future spack env support, but dev build doesn't help me keep my current workflow.

I am not against folks using the dev build approach if that works for their dev workflow, but I would like to be to restore the fake package workflow, given it has been successful since the beginning of uberenv.

cyrush · 2021-03-15T19:13:17Z

also resolves:

#47

cyrush · 2021-03-15T19:32:05Z

@kennyweiss @white238
the non dev build (legacy) path is now only triggered by theuberenv_package_name json entry, there is no command line arg. Is that sufficiently hidden to move forward?

white238 · 2021-03-15T19:40:46Z

Is there something else uberenv_package_name does than just force install vs dev-build? If so why not have the option be a boolean that does just that? (will look over PR after I eat my food)

cyrush · 2021-03-15T19:44:41Z

it specifies overridden package name uberenv-mypackage

…ke package

cyrush · 2021-03-16T19:23:24Z

refactored input options to provide a build-mode arg and build_mode json entry.

These options allow us to hand a specific build mode string down to the ubrerenv instances.

refactored spack instance logic to route this into self.build_mode with dev-build being the default.

Supported for spack are, dev-build, uberenv-pkg, and install. With this change folks can easily choose between dev build or uberenv pkg, via the json file, overridden by the command line arg.

kennyweiss · 2021-03-16T19:56:41Z

uberenv.py

+                      dest="build_mode",
+                      default=None,
+                      help="set mode used to build third party dependencies "
+                           "(spack options: 'dev-build' 'uberenv-pkg')")


@cyrush -- a comment that you posted indicates that install is also an option. Should it be added to this help line as well?

This seems like a much cleaner way to expose this functionality!

Looking through the code, it appears that install will be used if one does not set --build-mode,
so, perhaps the help should indicate that?

It's actually more complex than that :-(

The default build_mode should effectively be dev-build

The reason why it's complex has to do with the logic for command line args vs json overrides.

There is no one place for defaults, if you add a default in the command line arg def -- it will always override the json key (which isn't what we want).

kennyweiss

Thanks @cyrush -- this is a lot cleaner than the previous version.

I think the logic for setting up the type of build is getting too complex and hard to follow, so we should consider if we can refactor that in the future.

It will be really helpful to have some automated CI tests on the uberenv user codes to support that refactor (as @adrienbernede is planning)

kennyweiss · 2021-03-16T20:02:20Z

uberenv.py

+                      dest="build_mode",
+                      default=None,
+                      help="set mode used to build third party dependencies "
+                           "(spack options: 'dev-build' 'uberenv-pkg')")


Looking through the code, it appears that install will be used if one does not set --build-mode,
so, perhaps the help should indicate that?

cyrush · 2021-03-16T21:14:30Z

uberenv.py

-            raise
+            if not optional:
+                print("ERROR: '{0}' must at least be defined in project.json".format(setting))
+                raise
        else:


i think this else needs to go

adrienbernede · 2021-03-16T21:49:02Z

Thanks @cyrush -- this is a lot cleaner than the previous version.

I think the logic for setting up the type of build is getting too complex and hard to follow, so we should consider if we can refactor that in the future.

It will be really helpful to have some automated CI tests on the uberenv user codes to support that refactor (as @adrienbernede is planning)

There is a CI setup for Uberenv. It triggers a pipeline in both Serac and Conduit to check compatibility with the new Uberenv script. Watch me...

adrienbernede · 2021-03-16T21:49:06Z

LGTM

adrienbernede · 2021-03-16T21:58:51Z

And it will fail miserably for various reasons :(

cyrush · 2021-03-17T20:25:27Z

@adrienbernede is there any good info to pull from the gitlab tests?
I'd like to get this in, but I want to make sure this doesn't break anything in serac.

adrienbernede · 2021-03-17T20:30:48Z

The problem in Serac is that I just don't have access to their bank. Although I think they were OK with granting me access.

adrienbernede · 2021-03-17T20:31:08Z

I'll see if I can get this to work.

adrienbernede · 2021-03-17T22:05:57Z

I can confirm that this does not break Serac CI.

cyrush · 2021-03-17T22:19:21Z

thanks @adrienbernede !

white238 · 2021-03-17T22:24:10Z

uberenv.py

@@ -151,6 +151,14 @@ def parse_args():
                      default=None,
                      help="override the default package name")

+    # uberenv tpl build mode
+    parser.add_option("--build-mode",


spack-build-mode?

i made this change

uberenv.py

white238 · 2021-03-17T22:27:42Z

uberenv.py

+        spack_dir = self.dest_spack
+        cmd = pjoin(spack_dir,"bin","spack")
+        cmd += " python "
+        cmd += '-c "import sys; print(sys.executable);"'


uberenv.py

cyrush added 7 commits January 21, 2021 14:10

experiment

97c2e4e

experiment

f45f983

more experimenting

92d2a2c

fix

b412ee3

fix

1243c94

add -j opt, compromise to control host config dest

f7fe284

re-quiet the dev build

654727a

adrienbernede reviewed Jan 27, 2021

View reviewed changes

cyrush added 4 commits March 10, 2021 08:46

Merge branch 'master' into task/2020_01_cyrush_explore

dc4a1e8

restore fake pkg support

ff753c2

typo

b3a6d41

use detailed info

550f757

white238 reviewed Mar 10, 2021

View reviewed changes

kennyweiss reviewed Mar 11, 2021

View reviewed changes

fixes, refactor non dev build logic

4353b80

add display of both uberenv and spack python execs

4ae1b9d

relegate uberenv-package-name to json only option

c90dd7c

add a build mode option as a cleaner path to support dev-build and fa…

6d82e8e

…ke package

kennyweiss reviewed Mar 16, 2021

View reviewed changes

kennyweiss approved these changes Mar 16, 2021

View reviewed changes

cyrush commented Mar 16, 2021

View reviewed changes

cyrush added 2 commits March 16, 2021 14:21

stray else

12178ec

better description and comments

941ecf9

white238 reviewed Mar 17, 2021

View reviewed changes

uberenv.py Show resolved Hide resolved

white238 reviewed Mar 17, 2021

View reviewed changes

uberenv.py Show resolved Hide resolved

white238 approved these changes Mar 17, 2021

View reviewed changes

improvements suggested in PR

9a71ca8

cyrush merged commit 92c4cb4 into master Mar 17, 2021

cyrush deleted the task/2020_01_cyrush_explore branch March 17, 2021 23:32

joshessman-llnl added a commit that referenced this pull request Apr 26, 2021

fix: revert changes from #46, dev-build mode guards

54cf699

joshessman-llnl added a commit that referenced this pull request May 3, 2021

fix: revert changes from #46, dev-build mode guards

2ac9825

cyrush mentioned this pull request Jun 14, 2022

uberenv should report the version of python it is using and the version that spack is using. #47

Closed

white238 mentioned this pull request Nov 8, 2022

Move to Spack Environments #93

Closed

2021-01 cyrush workarounds and tweaks #46

2021-01 cyrush workarounds and tweaks #46

Conversation

cyrush commented Jan 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush Jan 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

white238 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush commented Mar 11, 2021

cyrush commented Mar 15, 2021

cyrush commented Mar 15, 2021 • edited Loading

white238 commented Mar 15, 2021

cyrush commented Mar 15, 2021

cyrush commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienbernede commented Mar 16, 2021

adrienbernede commented Mar 16, 2021

adrienbernede commented Mar 16, 2021

cyrush commented Mar 17, 2021

adrienbernede commented Mar 17, 2021

adrienbernede commented Mar 17, 2021

adrienbernede commented Mar 17, 2021

cyrush commented Mar 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrush Jan 27, 2021 •

edited

Loading

white238 Mar 10, 2021 •

edited

Loading

cyrush Mar 11, 2021 •

edited

Loading

cyrush commented Mar 15, 2021 •

edited

Loading