Fixes, Improvements and CI #21

adrienbernede · 2020-03-10T20:11:34Z

Fixes some undesired behavior introduced lately (see LLNL/conduit#502).

Also introduce some robustness improvements:

Check if a spec is already installed.
Ability to use the spec hash to accurately retrieve information (e.g. spack install path).

Add CI triggering Gitlab CI pipeline in Conduit and Serac.

Recent change:

Roll back of sexe() implementation after feedback.

adrienbernede · 2020-03-10T20:12:26Z

LGTM

adrienbernede · 2020-03-10T20:19:59Z

LGTM

adrienbernede · 2020-03-10T20:36:03Z

LGTM

adrienbernede · 2020-03-10T20:38:05Z

LGTM

adrienbernede · 2020-03-10T20:39:59Z

LGTM

adrienbernede · 2020-03-11T00:16:29Z

LGTM

kennyweiss

Leaving this as a comment since the PR is still labeled a WIP.

.gitlab-ci.yml

kennyweiss · 2020-03-11T22:46:45Z

.gitlab-ci.yml

+  variables:
+    UPDATE_UBERENV: $CI_COMMIT_REF_NAME
+  trigger:
+    project: radiuss/conduit


@adrienbernede -- what is the relationship between conduit and the uberenv CI plan?
Is this the first of several triggers (e.g. for other radiuss projects)?

I’m having a hard time validating Uberenv for every projects using it, and there are not so many.
This is both because there is no test in Uberenv per se, and because updating and testing it in other project is not automated. [EDIT: not blaming anyone]

What this does is to trigger conduit CI pipeline in Gitlab, asking for uberenv to be updated.
How it works:
In conduit, I added a script in (GitLab) CI that will be used only if UPDATE_UBERENV is set. The value is supposed to be the branch name in Uberenv repo.
Now if I trigger Conduit pipeline with this variable, Conduit install will be tested with the specified version of uberenv. I intend to do the same for other projects using Uberenv, but the mechanism can be use to test newer version of spack, or cross-project integration in general.

Note: branch will be set to master once feature/uberenv_ci will be merge in conduit. This PR is thus connected to LLNL/conduit#517

kennyweiss · 2020-03-11T22:51:41Z

uberenv.py

@@ -67,18 +67,30 @@
 from os.path import join as pjoin


-def sexe(cmd,ret_output=False,echo = False):
+def sexe(cmd, ret_output=False, print_output=True, echo=False):


I think this function has grown to the extent that it needs better documentation on the different options and how they interact.

Also, do we want print_output=True by default? Especially since this changes the default behavior.

I have maybe a hundred copies of this function :-)

for this use case:
if folks want to print the output, I would recommend using ret_output=True, and then printing the result outside of the this function.

Maybe I am just very superstitious, but not using res = p.communicate()[0] has deadlocked for me in the distant past.

OK, here is why I changed this function:

With the original implementation, the output of the function was either printed or returned. Making it almost* impossible to get both. This was a problem because I wanted to parse the output of spack install, but still show this output to the user.
"almost impossible" because one solution was to print the output after sexe returned it. But I’m pretty sure no one wants to get the full spack install output printed at once after the full install is complete... correct me if I’m wrong.

So with this implementation, I can both print the output in the terminal while it happens, and return it to parse for errors.

@kennyweiss by default, sexe used to print the output in terminal, and not return it, so I think we do want print_output=True in order to match the original behavior.

@cyrush there are more advanced implementations for this, maybe more reliable? I chose to use something I understand well, at least as long as it works.

@adrienbernede is it b/c you want to stream the output as well as parse it, as opposed to capture it all and print it?

I agree you will need to do that for such a long running command, however parsing the spack output of such a large operation might be brittle and a source of woes in the future.

Can you run a separate command to check if it is already installed before the install command? You might be able to get a more robust answer from a spack return code for a more specific command. If this is possible, then I don't think you need this change?

Overall your change to sexe looks ok - but I know that p.communicate()[0] is robust from years of use. I really don't want uberenv to deadlock on me :-)

Sorry for so much guidance on what seems like such a minor change, but I want to apply caution.

@adrienbernede is it b/c you want to stream the output as well as parse it, as opposed to capture it all and print it?

Yes, that’s exactly what I meant.

Can you run a separate command to check if it is already installed before the install command? You might be able to get a more robust answer from a spack return code for a more specific command. If this is possible, then I don't think you need this change?

It’s in my plan to make Uberenv more robust using the hash of the desired spec to guide subsequent spack commands. I want Uberenv to ask spack to concretize the desired spec, and then use the generated spec to check for path/already installed etc.
(e.g. find_spack_pkg_path isn’t robust as-is)

So once I have this improvement, I will be able to check in the spec is already installed, and won’t need this implementation of sexe() anymore.

But first, I want a reliable way to check my changes. It’s so easy to leave a bug in this process.

Sorry for so much guidance on what seems like such a minor change, but I want to apply caution.

No worries, I’d rather have guidance now than later :)

uberenv.py

adrienbernede

Reviewing myself...

adrienbernede · 2020-03-11T23:49:30Z

uberenv.py

@@ -67,18 +67,30 @@
 from os.path import join as pjoin


-def sexe(cmd,ret_output=False,echo = False):
+def sexe(cmd, ret_output=False, print_output=True, echo=False):


OK, here is why I changed this function:

With the original implementation, the output of the function was either printed or returned. Making it almost* impossible to get both. This was a problem because I wanted to parse the output of spack install, but still show this output to the user.
"almost impossible" because one solution was to print the output after sexe returned it. But I’m pretty sure no one wants to get the full spack install output printed at once after the full install is complete... correct me if I’m wrong.

So with this implementation, I can both print the output in the terminal while it happens, and return it to parse for errors.

@kennyweiss by default, sexe used to print the output in terminal, and not return it, so I think we do want print_output=True in order to match the original behavior.

@cyrush there are more advanced implementations for this, maybe more reliable? I chose to use something I understand well, at least as long as it works.

uberenv.py

adrienbernede · 2020-03-12T00:03:05Z

.gitlab-ci.yml

+  variables:
+    UPDATE_UBERENV: $CI_COMMIT_REF_NAME
+  trigger:
+    project: radiuss/conduit


I’m having a hard time validating Uberenv for every projects using it, and there are not so many.
This is both because there is no test in Uberenv per se, and because updating and testing it in other project is not automated. [EDIT: not blaming anyone]

What this does is to trigger conduit CI pipeline in Gitlab, asking for uberenv to be updated.
How it works:
In conduit, I added a script in (GitLab) CI that will be used only if UPDATE_UBERENV is set. The value is supposed to be the branch name in Uberenv repo.
Now if I trigger Conduit pipeline with this variable, Conduit install will be tested with the specified version of uberenv. I intend to do the same for other projects using Uberenv, but the mechanism can be use to test newer version of spack, or cross-project integration in general.

Note: branch will be set to master once feature/uberenv_ci will be merge in conduit. This PR is thus connected to LLNL/conduit#517

.gitlab-ci.yml

adrienbernede · 2020-04-22T03:58:32Z

LGTM

adrienbernede · 2020-04-22T19:35:45Z

LGTM

TODO: restriction not perfect for find_pkg, needs hash.

The goal is to trigger (dowstream) conduit pipeline with updated uberenv.

uberenv.py

adrienbernede · 2020-04-22T21:10:21Z

@kennyweiss @cyrush
This is open for review and suggestion.
I could document the CI addition. Changes in code should be almost transparent to usage, since that was the goal.

adrienbernede

typo

uberenv.py

kennyweiss

Looks good @adrienbernede

Please make sure to update the readthedocs documentation for the new capabilities / options.

uberenv.py

kennyweiss · 2020-04-26T19:35:48Z

uberenv.py

+        res, out = sexe(spec_cmd, ret_output=True, echo=True)
+        print(out)
+
+        for line in out.split("\n"):


Maybe add a comment above the for line to say what this is doing ?
Looks like it is looking for duplicates?

kennyweiss · 2020-04-26T19:38:01Z

uberenv.py


+        if not "use_install" in self.opts:


Is the use_install option documented?

For some reason I don’t remember, I added this as an option but only used and assignable internally, so not really an option.
I’m making it a SpackEnv field.

uberenv.py

kennyweiss · 2020-04-26T19:45:57Z

.gitlab-ci.yml

+  variables:
+    UPDATE_UBERENV: $CI_COMMIT_REF_NAME
+  trigger:
+    project: radiuss/conduit


If I understand this correctly, these trigger gitlab CI builds for conduit.

Will there be any feedback for the uberenv PR about the success of the gitlab task? E.g. block the PR if it fails?

Yes that’s what it is.

There is already a feedback, but since I used and abused of force push, it’s not obvious where to find it:

The last commit for which I added the "LGTM" sign-off has a testing status attached to it linking to the Gitlab corresponding CI pipeline.
35a4145

This feedback could be used on Github side to prevent from merging. Of course, updating Uberenv in Serac and Conduit may not be straightforward for known reasons and the test would then fail.

adrienbernede · 2020-04-27T20:37:54Z

LGTM

adrienbernede · 2020-04-29T01:52:20Z

LGTM

adrienbernede · 2020-04-29T16:18:29Z

@cyrush @kennyweiss @white238 May I merge the branch? I'm used to double review. :)

cyrush · 2020-04-29T16:28:14Z

overall looks good.

I checked out the gitlab outputs, are there some hidden errors in there?
cp: cannot stat '/usr/WS2/bernede1/gitlab-runner/builds/zkQzXxmM/0/gitlab/radiuss/conduit/scripts/uberenv/packages/*': No such file or directory

Are things getting applied as we expect?

kennyweiss

Looks good to me, but please look into the logged warnings that @cyrush noted.

adrienbernede · 2020-04-29T20:09:29Z

LGTM

adrienbernede · 2020-04-29T20:38:59Z

This is a good catch leading to an interesting point:
Uberenv CI is currently configured to trigger CI on a very specific branch of Conduit (an open PR), because Conduit do not have Gitlab CI in master. The branch I was pointing to was a bit outdated and did not contain any spack packages, hence the error, which has no critical consequence.

I keep force pushing micro changes to be able to trigger pipeline again.

adrienbernede changed the title ~~Fixes, Improvments and CI~~ [WIP] Fixes, Improvments and CI Mar 10, 2020

adrienbernede force-pushed the fix/original_behavior branch from 4f9da26 to d48c789 Compare March 10, 2020 20:19

adrienbernede force-pushed the fix/original_behavior branch from 0886e64 to 9efe486 Compare March 10, 2020 20:37

adrienbernede force-pushed the fix/original_behavior branch from 9efe486 to f6d496c Compare March 10, 2020 20:39

kennyweiss reviewed Mar 11, 2020

View reviewed changes

adrienbernede commented Mar 12, 2020

View reviewed changes

adrienbernede force-pushed the fix/original_behavior branch from e6cebe3 to 6795633 Compare March 12, 2020 15:30

adrienbernede changed the title ~~[WIP] Fixes, Improvments and CI~~ [WIP] Fixes, Improvements and CI Mar 19, 2020

adrienbernede added 2 commits April 21, 2020 11:44

Updating with feedback from conduit

8fb1aaa

Addressing already installed package case

85a886e

adrienbernede force-pushed the fix/original_behavior branch from 6795633 to eb01f2a Compare April 22, 2020 03:57

adrienbernede force-pushed the fix/original_behavior branch from eb01f2a to 35a4145 Compare April 22, 2020 19:31

adrienbernede changed the title ~~[WIP] Fixes, Improvements and CI~~ Fixes, Improvements and CI Apr 22, 2020

adrienbernede added 4 commits April 22, 2020 13:49

Restrict spec in find_pkg and activation

e2960e6

TODO: restriction not perfect for find_pkg, needs hash.

Add Gitlab pipeline in uberenv:

0665ff2

The goal is to trigger (dowstream) conduit pipeline with updated uberenv.

Adding trigger of serac CI with uberenv update

150c4ca

Fix minor bug: incorrect variable scope

5c6a915

adrienbernede force-pushed the fix/original_behavior branch from 35a4145 to 5c6a915 Compare April 22, 2020 20:49

adrienbernede commented Apr 22, 2020

View reviewed changes

uberenv.py Outdated Show resolved Hide resolved

adrienbernede commented Apr 22, 2020

View reviewed changes

uberenv.py Outdated Show resolved Hide resolved

adrienbernede commented Apr 22, 2020

View reviewed changes

uberenv.py Show resolved Hide resolved

adrienbernede commented Apr 24, 2020

View reviewed changes

uberenv.py Outdated Show resolved Hide resolved

kennyweiss approved these changes Apr 26, 2020

View reviewed changes

kennyweiss added the CI Related to Continuous Integration (CI) label Apr 26, 2020

adrienbernede added 2 commits April 27, 2020 10:07

Addressing review comments

c27e8fa

Comment about extra check due to spack issue

0b51ab6

adrienbernede force-pushed the fix/original_behavior branch from 36419e0 to 300b721 Compare April 29, 2020 01:52

kennyweiss approved these changes Apr 29, 2020

View reviewed changes

adrienbernede mentioned this pull request Apr 29, 2020

Update Uberenv and introduce Gitlab CI [feature/uberenv_ci] LLNL/conduit#517

Closed

Using spec hash instead of given spec (robustness)

df5fd04

adrienbernede force-pushed the fix/original_behavior branch from 300b721 to df5fd04 Compare April 29, 2020 20:09

adrienbernede merged commit f688443 into master Apr 29, 2020

white238 deleted the fix/original_behavior branch August 23, 2021 23:41

Fixes, Improvements and CI #21

Fixes, Improvements and CI #21

Conversation

adrienbernede commented Mar 10, 2020 • edited

adrienbernede commented Mar 10, 2020

adrienbernede commented Mar 10, 2020

adrienbernede commented Mar 10, 2020

adrienbernede commented Mar 10, 2020

adrienbernede commented Mar 10, 2020

adrienbernede commented Mar 11, 2020

kennyweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienbernede Mar 12, 2020 • edited

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

adrienbernede left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrienbernede Mar 12, 2020 • edited

Choose a reason for hiding this comment

adrienbernede commented Apr 22, 2020

adrienbernede commented Apr 22, 2020

adrienbernede commented Apr 22, 2020

adrienbernede left a 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

adrienbernede Apr 27, 2020 • edited

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

adrienbernede commented Apr 27, 2020

adrienbernede commented Apr 29, 2020

adrienbernede commented Apr 29, 2020 • edited

cyrush commented Apr 29, 2020

kennyweiss left a comment

Choose a reason for hiding this comment

adrienbernede commented Apr 29, 2020

adrienbernede commented Apr 29, 2020

adrienbernede commented Mar 10, 2020 •

edited

adrienbernede Mar 12, 2020 •

edited

adrienbernede Mar 12, 2020 •

edited

adrienbernede Apr 27, 2020 •

edited

adrienbernede commented Apr 29, 2020 •

edited