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

reinstate OSX Github Action tests #2110

Merged
merged 17 commits into from Jun 24, 2021
Merged

reinstate OSX Github Action tests #2110

merged 17 commits into from Jun 24, 2021

Conversation

valeriupredoi
Copy link
Contributor

@zklaus here is a sandbox for you to test about the OSX conda-forge recipe solutions for OSX (if you don't actually have an OSX machine where you can test hands-on)

@zklaus
Copy link
Contributor

zklaus commented Mar 30, 2021

Thanks @valeriupredoi, this looks nice. However, at the moment I am mostly looking at ESMValCore for conda-forge. Could you help me setup a similar test there?

@valeriupredoi
Copy link
Contributor Author

we have them and they work well (well, unless we hit the dreaded SegFaults, which I am about to fix finally) - @bvreede is our new resident OSX guru so I reckon she's the best person to ask when you get stuck in OSX lands 😃

@valeriupredoi
Copy link
Contributor Author

speaking of - maybe @bvreede can haz a look at this too 😁

@stefsmeets stefsmeets changed the title reinstae OSX Github Action tests reinstate OSX Github Action tests Mar 31, 2021
@bvreede
Copy link
Contributor

bvreede commented Mar 31, 2021

Sorry for hi-jacking the PR, @valeriupredoi, hope you don't mind!

The issue is with R, ncl and julia; installing these on mac with conda does not work with the given environment file. In addition, while I fixed the tests for osx on esmvalcore, I did not get around to esmvaltool yet 😬 so I'm adding this onto the PR, hope that is ok?
I have made a separate environment file that is really only used to generate the conda env for GA. I'm working right now (with @stefsmeets) to label the offending tests so they do not run on an osx platform. Converting this back to draft until it's functional :)

@bvreede bvreede marked this pull request as draft March 31, 2021 14:11
@valeriupredoi
Copy link
Contributor Author

@bvreede au contraire, thank you very much for hijacking the PR - you and Stef are doing a great job, cheers muchly 🍺

@stefsmeets stefsmeets self-requested a review April 1, 2021 11:20
@stefsmeets
Copy link
Contributor

Flake8: /home/runner/work/ESMValTool/ESMValTool/tests/integration/test_diagnostic_run.py:79:2: N816 variable 'recipe_R' in global scope should not be mixedCase

@bvreede
Copy link
Contributor

bvreede commented Apr 1, 2021

Right, that was the lone leftover, @stefsmeets 😅
Should run smoothly now, fingers crossed [probably jinxing it right now, oops]!
I'll revert this PR back to ready for review!

@bvreede bvreede marked this pull request as ready for review April 1, 2021 14:09
@bvreede
Copy link
Contributor

bvreede commented Apr 1, 2021

The upshot of this somewhat evasive "fix" is that there really is no conda-based compatibility for the entire esmvaltool suite with mac osx, at least not that I can find right now. OSX-users will need to work with esmvaltool-python for now, and this would be one way that we can still run all python-based tests on osx. I'll see if I can figure out a good way to use the other tools on a mac, but this will probably not be with conda.

I'll be adjusting the docs accordingly. That PR is coming up :) [edit: it is ready at #2115 ]

@bvreede
Copy link
Contributor

bvreede commented Apr 1, 2021

Right, I overlooked something, as expected.

All osx test actions fail with this error:

E   xgboost.core.XGBoostError: XGBoost Library (libxgboost.dylib) could not be loaded.
E   Likely causes:
E     * OpenMP runtime is not installed (vcomp140.dll or libgomp-1.dll for Windows, libomp.dylib for Mac OSX, libgomp.so for Linux and other UNIX-like OSes). Mac OSX users: Run `brew install libomp` to install OpenMP runtime.
E     * You are running 32-bit Python on a 64-bit OS

Locally, I can run brew install libomp and presto, the error disappears. I don't know how to do this for github actions though.

Help appreciated!

@stefsmeets
Copy link
Contributor

stefsmeets commented Apr 2, 2021

Locally, I can run brew install libomp and presto, the error disappears. I don't know how to do this for github actions though.

You can install stuff on the GA machine by adding the shell command, but I doubt brew is available.

Can we skip the entire thing using skipif or as xfail (pytest.marks.xfail) with the xgboost.core.XGBoostError error?

EDIT: apparently brew is available, can you try adding this step:

      - name: Install libomp with homebrew
        run: brew install libomp

@bvreede
Copy link
Contributor

bvreede commented Apr 7, 2021

Well that was easier than expected 😅 -- thanks a bunch, @stefsmeets!

Should we recommend the installation of libomp for macosx users? The error appears with diag_scripts/mlr/models/gbr_xgboost.py so an alternative could be to put a specific message on the recipe itself (CC @schlunma).

@schlunma
Copy link
Contributor

schlunma commented Apr 7, 2021

I've never used a Mac before; for me the code worked without any additional installation 😅

Feel free to add a note to the recipe, but I think the error message itself is really clear and tells you exactly what to do, so this might not be necessary.

Thanks for looking into this!

@bvreede
Copy link
Contributor

bvreede commented Apr 7, 2021

Thanks @schlunma — that is a good point and works for me :)

@stefsmeets Would you be able to give this a review still?

Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

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

Hi @bvreede , nice work! Looks good to me, just a few minor things before I can approve 👍

@@ -5,6 +5,7 @@ on:
push:
branches:
- master
- reinstate_GAtest_OSX
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be removed before merging, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding branches to the workflow, I propose that we use a PR on master as a trigger to run tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, good point! 👍

.github/workflows/action-test.yml Outdated Show resolved Hide resolved
.github/workflows/action-test.yml Outdated Show resolved Hide resolved
tests/integration/test_diagnostic_run.py Outdated Show resolved Hide resolved
@stefsmeets stefsmeets self-requested a review April 8, 2021 09:04
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

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

Nice work @bvreede and @valeriupredoi , looks good to me! 🚀

@bvreede
Copy link
Contributor

bvreede commented Apr 8, 2021

Thanks a lot @stefsmeets — just to confirm, @valeriupredoi, this PR would invoke running this GA workflow (with 4 python versions x 2 OS) on every PR. It's a PITA because it takes forever, but it's much more thorough than the circleCI tests, and it makes sense to run this on a PR and not just on the master branch once changes are pushed, imho.

@valeriupredoi
Copy link
Contributor Author

sorry for being a delaying PITA myself, am back in work now and will have a looksee at this on Monday 🍺

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Apr 19, 2021

Thanks a lot @stefsmeets — just to confirm, @valeriupredoi, this PR would invoke running this GA workflow (with 4 python versions x 2 OS) on every PR. It's a PITA because it takes forever, but it's much more thorough than the circleCI tests, and it makes sense to run this on a PR and not just on the master branch once changes are pushed, imho.

Great job! I agree - I am always in favor of thorough testing vs fast push to master then fixing broken bits that were not picked up by local PR or branch tests. But I am afraid this will have to be decided by a number of other core tech people as well (I can't just be a benevolent dictator and push forward with this only to have to recall it later) so @bouweandela @jvegasbsc @nielsdrost what do you guys think - note that @bvreede was kind enough to include an OSX env file that completely sorts out the installation on OSX (not just an env creation but rather a correct and working install procedure). I am in favor of all the functionality here BTW 👍

@bouweandela
Copy link
Member

it makes sense to run this on a PR and not just on the master branch once changes are pushed

@bvreede Why do you think this makes sense? As far as I can see the required changes are small and only in files that most diagnostic contributors will never change, so for vast the majority of pull requests this would be only overhead.

Should we recommend the installation of libomp for macosx users?

Yes, otherwise some of the diagnostics do not work. Would it be possible to install it from conda instead of with brew for a more consistent installation experience?

Also, I see no mention of the special OS X conda environment file in the development installation instructions, but I assume it would be needed?

@info "Done"
"""),
}
recipe_py = """
Copy link
Member

Choose a reason for hiding this comment

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

Please follow PEP8 and use all caps names for global variables

@bvreede bvreede mentioned this pull request Jun 14, 2021
@bvreede
Copy link
Contributor

bvreede commented Jun 15, 2021

With apologies for the delay on this...

Thanks for the review @bouweandela! Your comments have been addressed:

  • global variables in the edited test are in caps
  • information about the installation of libomp with brew (unfortunately not possible via conda) is in the docs
  • as is information about the use of the environment_osx.yml for developers on osx
  • PR has been removed as an action trigger.

Note: the branch name will have to be removed from action-test.yml before merge!

@bvreede bvreede requested a review from bouweandela June 15, 2021 05:15
Copy link
Member

@bouweandela bouweandela left a comment

Choose a reason for hiding this comment

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

Thanks @bvreede for addressing my review comments. I think this is good to go as is, but if you like there are still some more ideas for improvement.

@@ -500,6 +510,14 @@ and run
This command installs many of the required dependencies from conda, including
the ESMValCore package and Python, R, and NCL interpreters.

**MacOSX note:** ESMValTool functionalities in Julia, NCL, and R are not
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is really true. Did you try

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for those suggestions, @bouweandela ! I will flag this and address that at a later point. Currently it is beyond the scope of this PR, but it would be good to assess in what way we can make ESMValTool compatible with OSX fully, with multi-language support.

tests/integration/test_diagnostic_run.py Outdated Show resolved Hide resolved
environment_osx.yml Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor Author

cheers for the review @bouweandela and many thanks for the great work @bvreede - one quick question before I start deleting the test branch from the GA workflows - do you guys still want to go about changes from Bouwe's comments or shall we address those in a future PR? I like the green ticks so am happy to merge now (well, it's me own PR but maybe Bouwe can merge). We'll have to examine all these checks in the very near future anyway, part of @zklaus idea of sitting down and going through all our automated tests workshop thingy 🍺

@bvreede
Copy link
Contributor

bvreede commented Jun 23, 2021

Hi @valeriupredoi, thanks! I am not working 100% at the moment & a bit stretched, but this was on my plan to address asap. I think in particular the comment about the developer documentation is important to address now. Whether and how ESMValTool could work on mac with ncl, R, and Julia is something that is still tbd, but is beyond the scope of this PR.

Give me 24h max and then you can merge?

@bvreede
Copy link
Contributor

bvreede commented Jun 24, 2021

Finished this up with @stefsmeets — it's good to go! I removed the branch from the GA triggers in my latest commit, so it should be ready to merge if you want!

Since I removed the branch from triggers, the actions no longer show up below, but here's the latest GA tests run.

@valeriupredoi
Copy link
Contributor Author

excellent, great many thanks @bvreede 🍺

@valeriupredoi
Copy link
Contributor Author

and @stefsmeets 🍺

@valeriupredoi valeriupredoi merged commit 7943ca7 into main Jun 24, 2021
@valeriupredoi valeriupredoi deleted the reinstate_GAtest_OSX branch June 24, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants