InVEST 64-bit workflow using conda with Python 3.8 #314

dcdenu4 · 2020-09-18T22:24:37Z

These changes update the build workflow to only 64-bit and add python 3.8 testing. The build workflow now uses conda almost exclusively to handle InVEST dependencies to the point that our hosted binaries from Gohlke are no longer being used. Python 3.8 binaries are not being built because Pyinstaller does not yet officially support python 3.8, though it seems to be coming in the next release.

Github workflow files

Drops support for x86 32 bit architecture
Uses conda environments exclusively where possible to manage dependencies
Uses bash shell exclusively where possible
Removes python 3.6 testing and adds python 3.8

InVEST spec and hooks

Since using conda for windows as well as mac, add spatiallib*.dll handling for windows
Grab conda environment via environment variable and remove hard coding where possible
With GDAL 3+ need to make sure that we bring along the PROJ folder and proj.db
Added PROJ_LIB environment variable setting to runtime hook

Makefile

Set bash as default shell for Windows thanks to github actions having a bash shell for Windows
Remove powershell / cmd commands where possible and replace with bash like equivalents
powershell was particularly flaky when doing this update, especially related to ignoring errors. It appeared that if a sub command wrote to standard out powershell would assume that meant an error and exit.

It would be great if people could download the installer and test models. I tested UCM since it had a shapely import and it ran okay.

References issue #4
References issue #1

removing coverage commands and putting a string around wild card omit allowed tests to pass / make to pass. Now try adding back to extra coverage commands to see which was the deal breaker.

dcdenu4 · 2020-10-02T17:39:21Z

Hey @dcdenu4 , I just noticed that the make env command for Windows hasn't really changed in this PR, probably because it's not being used anywhere in our workflows. But do you think it makes sense to update that to also use conda?

I'm asking because I'm working on the workbench build process - which needs to clone invest and setup a python environment and it would be great to re-use code from invest to do that, rather than duplicate yml config in two different repos. Basically, I think I have a use-case for make env and various other invest make recipes. What do you think?

Hey @davemfish I was just looking at this and I think it's a good avenue but will be a little tricky because of "activating" the environment depending on whether you are already in a conda environment (github workflow) or a developer in a terminal. It might even make more sense to have a invest-python-requirements make command. That or have some logic that checks to see if some conda env variables are set, or take in a parameter... All to say, I'm thinking of making this a separate issue and not tackling it in this PR? Let me know what you think.

Sorry, the tricky bit is using the make env command in the github workflows properly while also supporting use via command line from a developer and what they expect that command to do.

Update other workflows to use no mkl versions of numpy and scipy as well.

davemfish · 2020-10-02T18:54:06Z

Hey @davemfish I was just looking at this and I think it's a good avenue but will be a little tricky because of "activating" the environment depending on whether you are already in a conda environment (github workflow) or a developer in a terminal. It might even make more sense to have a invest-python-requirements make command. That or have some logic that checks to see if some conda env variables are set, or take in a parameter... All to say, I'm thinking of making this a separate issue and not tackling it in this PR? Let me know what you think.

Sorry, the tricky bit is using the make env command in the github workflows properly while also supporting use via command line from a developer and what they expect that command to do.

Okay, thanks for looking into it @dcdenu4 . I don't mind at all if it's pushed to a new issue & PR. I'll make an issue for the broader "invest build process should meet the needs of the invest-workbench build process" issue.

dcdenu4 · 2020-10-02T20:29:49Z

@davemfish @phargogh @emlys I just pushed a change to the Readme about make now requiring bash. The only thing I think left to look into here is the Windows binary signings. As I mentioned above, it appears to sign correctly. The recreation bug should also be fixed now. The NDR and SWY are being handled in a separate PR. Here's a link to the successful build prior to Readme change: https://github.com/dcdenu4/invest/suites/1280673421/artifacts/19848678

The way the regular expression was set up, it was grabbing `dist/invest/invest.exe` and not `dist/InVEST_[fork-tag-rev]_Setup.exe`.

davemfish

Hi @dcdenu4 , I found a few very minor things to note on the workflows here. And I think I found a powershell reference that is causing the mac builds to fail at the moment. Thanks for taking a look!

I also noticed several "Restore pip cache" steps in the workflows and I wonder if those can be purged give the very light usage of pip that is left. Or maybe it's just safer & better to leave them in.

davemfish · 2020-10-05T13:24:43Z

.github/workflows/binary-applications.yml

              with:
-                  python-version: ${{ env.PYTHON_VERSION }}
-                  architecture: ${{ env.PYTHON_ARCH }}
+                  activate-environment: winbin-env  # This is also hardcoded in Pyinstaller's invest.spec


Isn't the spec referring to os.environ['CONDA_PREFIX'] now? Maybe this comment is outdated.

Yep, these were leftover comments and have been removed.

davemfish · 2020-10-05T13:32:54Z

.github/workflows/binary-applications.yml

              uses: goanpeca/[email protected]
              with:
-                  activate-environment: mac-env  # This is also hardcoded in Pyinstaller's invest.spec
+                  activate-environment: macbin-env  # This is also hardcoded in Pyinstaller's invest.spec


Same comment as above, I think this code comment may be outdated.

davemfish · 2020-10-05T13:33:39Z

.github/workflows/build-and-test.yml

-                exclude:
-                    - os: macos-latest
-                      python-architecture: x86
+                      numpy: "numpy=1.15"  # fuzzy assertion in conda is single '='


Thanks, I never knew that!

davemfish · 2020-10-05T13:39:27Z

.github/workflows/build-and-test.yml

-                  pip install toml twine ${{ matrix.numpy }}
-                  pip install $(python -c "import toml;print(' '.join(toml.load('pyproject.toml')['build-system']['requires']))")
+                  conda install ${{ matrix.numpy }}
+                  python -m pip install scipy


It may not matter much when it's installed, but is scipy really a build dependency?

Good catch, fixed here: b66ed3b

davemfish · 2020-10-05T13:54:09Z

Makefile

-	$(BASHLIKE_SHELL_COMMAND) "$(PYTHON) -m pip freeze --all > $(INVEST_BINARIES_DIR)/package_versions.txt"
+	# wrapping conda command in powershell since some windows bash shells
+	# don't have access to conda command.
+	powershell.exe -Command "conda list --export > $(INVEST_BINARIES_DIR)/package_versions.txt"


I think this powershell reference is what is causing the Mac builds to fail right now.

Oh yea, oops. Looks like the homebrew Mac bugs have been fixed though!

Thanks for spotting this. I fixed it here: fa4e458 by setting a CONDA environment variable that calls conda for mac/nix and conda.bat for windows.

This is because Windows is really looking for conda.bat and from a bash shell doesn't recognize conda without the extension

dcdenu4 · 2020-10-05T20:23:40Z

I also noticed several "Restore pip cache" steps in the workflows and I wonder if those can be purged give the very light usage of pip that is left. Or maybe it's just safer & better to leave them in.

Good question @davemfish . Right along with that, should we be handling a Conda cache somewhere as well? What do you think @phargogh ?

I think I've answered all other questions and did fix the windows signing issue (spoiler, I was signing dist/invest/invest.exe instead of dist/InVEST_*Setup.exe. Please let me know if I missed something.

The link to test the Windows installer is here:
https://github.com/dcdenu4/invest/suites/1292284363/artifacts/20167505

phargogh · 2020-10-05T20:40:16Z

should we be handling a Conda cache somewhere as well? What do you think @phargogh ?

I implemented the pip cache because it was (at the time) strongly recommended in the github actions docs. In practice, I never got the cache to work reliably such that we would actually use our previously cached items. And it took quite a while to get to that point!

In our case, and given how much time caching took up to implement, I'd suggest that we pursue caching only if we know that a significant amount of runtime is spent downloading artifacts from conda or pypi, and the speedup promised from avoiding these downloads outweighs the cost of unzipping the cache.

dcdenu4 · 2020-10-06T17:28:48Z

I implemented the pip cache because it was (at the time) strongly recommended in the github actions docs. In practice, I never got the cache to work reliably such that we would actually use our previously cached items. And it took quite a while to get to that point!

In our case, and given how much time caching took up to implement, I'd suggest that we pursue caching only if we know that a significant amount of runtime is spent downloading artifacts from conda or pypi, and the speedup promised from avoiding these downloads outweighs the cost of unzipping the cache.

Hey @davemfish and @phargogh , as I mentioned on the call today, I think we should punt on any changes to caching as outside this PRs scope. I'll make an issue with some of the discussion we've had. (#326)

davemfish

Latest changes look good to me!

emlys

Looks good! Just had a question about possible unused RM and RMDIR variables

emlys · 2020-10-07T16:47:12Z

Makefile

@@ -248,10 +252,10 @@ $(DIST_DIR)/natcap.invest%.zip: | $(DIST_DIR)
 # on Windows as the .exe extension is assumed.
 binaries: $(INVEST_BINARIES_DIR)
 $(INVEST_BINARIES_DIR): | $(DIST_DIR) $(BUILD_DIR)
-	-$(RMDIR) $(BUILD_DIR)/pyi-build
-	-$(RMDIR) $(INVEST_BINARIES_DIR)
+	-rm -r $(BUILD_DIR)/pyi-build


Can these use the RMDIR variable defined on lines 27/52?

Fixed here: f8a92c7

emlys · 2020-10-07T16:48:26Z

Makefile

@@ -269,11 +273,11 @@ $(APIDOCS_ZIP_FILE): $(APIDOCS_HTML_DIR)
 userguide: $(USERGUIDE_HTML_DIR) $(USERGUIDE_ZIP_FILE)
 $(USERGUIDE_HTML_DIR): $(GIT_UG_REPO_PATH) | $(DIST_DIR)
 	$(MAKE) -C doc/users-guide SPHINXBUILD="$(PYTHON) -m sphinx" BUILDDIR=../../build/userguide html
-	-$(RMDIR) $(USERGUIDE_HTML_DIR)
+	-rm -r $(USERGUIDE_HTML_DIR)


Same here, it looks like the RMDIR variable is unused

Fixed here: f8a92c7

phargogh

Looks great to me ... thanks a ton for shepherding this build process into the future @dcdenu4 !

phargogh · 2020-10-09T20:13:50Z

Makefile

-jenkins:
-	$(JENKINS_BUILD_SCRIPT)
-
-jenkins_test_ui: env
-	$(MAKE) PYTHON=$(ENV_SCRIPTS)/python test_ui
-
-jenkins_test: env $(GIT_TEST_DATA_REPO_PATH)
-	$(MAKE) PYTHON=$(ENV_SCRIPTS)/python test
-


Fare thee well, jenkins.

phargogh · 2020-10-09T20:14:17Z

README.rst

+.. note::
+    The ``make`` commands for InVEST require a BASH shell environment. Windows
+    users can use Git Bash within the Git for Windows suite. More infomration
+    can be found at https://gitforwindows.org


Ah, excellent. Thanks for noting this!

phargogh · 2020-10-09T20:18:31Z

@dcdenu4 I just resolved the merge conflict on HISTORY. The one CI failure looks like the homebrew issue, the one that's ultimately up to github ... does that sound right to you?

phargogh · 2020-10-09T20:53:54Z

Looks like the mac issue must have been related to the github actions thing as we suspected. Looks good! I'll merge now.

phargogh · 2020-10-09T20:54:16Z

🎉

dcdenu4 added 30 commits September 2, 2020 16:01

Remove python 3.6 and x86 builds

4700561

Switch windows build test to use conda

1fecf32

Remove pypi hosted sources for runtime dep

004c3fc

Adding debug flags to make commnad

7f29b0c

Use conda but revert to using pip

fad7425

Only call one test to speed up 'testing'

083e5db

Changes to windows build setup

b4560b0

string around omit for coverage. remove coverage ext

edf29cb

Actually remove coverage extras and add quotes

331b947

Try adding back other coverage commands

65cd5f3

removing coverage commands and putting a string around wild card omit allowed tests to pass / make to pass. Now try adding back to extra coverage commands to see which was the deal breaker.

Debugging GCS deploy error

71a4d12

Debug GCS deploy with print statements

b46eed5

More debuggin flags and echos

82ed7b5

Wrap deploy command in string

7e0229c

See debug with original call

c5f0e50

debug

bbf82eb

adding quotes around GCS deploy calls

ce772d1

Removing debugging statements / code

153f69c

Add python 3.8 and use conda instead of pip

9006a53

just use one test for speed

8f6b28c

forgot the dot py

8fcaf2a

update ui-tests to use conda

70abb81

install pytest, use conda for validate test

249d4bc

install requests

ab2ff4e

Use bash shell and new env

8cb1548

reduce redundancies for mac and windows

46f5057

64 bit binary versions

d05a236

no-index for wheel install

00c0d9e

fix windows binary python ver bug

dd59133

Don't do any shapely dll stuff

a8b04d0

set pygeo as pip only to avoid conflicts

7f92ddf

Try not explicitly install scipy

c3f3e12

Update other workflows to use no mkl versions of numpy and scipy as well.

dcdenu4 added 2 commits October 2, 2020 15:34

Fix bug with py 38 numpy version and pyinstaller

5144576

Update readme to reflect make cmds requiring bash

0de6051

dcdenu4 requested review from phargogh and davemfish October 2, 2020 20:29

Updating signing binary regular expression

677e9bd

The way the regular expression was set up, it was grabbing `dist/invest/invest.exe` and not `dist/InVEST_[fork-tag-rev]_Setup.exe`.

davemfish requested changes Oct 5, 2020

View reviewed changes

dcdenu4 added 3 commits October 5, 2020 15:06

set conda cmd variable

fa4e458

This is because Windows is really looking for conda.bat and from a bash shell doesn't recognize conda without the extension

Remove comments about hard coded conda paths

1994bcf

Remove scipy install

b66ed3b

dcdenu4 requested a review from davemfish October 5, 2020 20:23

davemfish approved these changes Oct 6, 2020

View reviewed changes

emlys approved these changes Oct 7, 2020

View reviewed changes

dcdenu4 added 2 commits October 8, 2020 06:32

Consistently use RM RMDIR vars

f8a92c7

add 3.8 support to setup.py, drop 3.6

c0c292b

dcdenu4 mentioned this pull request Oct 9, 2020

Pyinstaller does not support Python 3.8 #330

Closed

Fixing history indentation

eb73e81

phargogh approved these changes Oct 9, 2020

View reviewed changes

Merge branch 'release/3.9' into task/FINAL-py38-conda-64bit-builds

93524da

phargogh merged commit 3648680 into natcap:release/3.9 Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InVEST 64-bit workflow using conda with Python 3.8 #314

InVEST 64-bit workflow using conda with Python 3.8 #314

dcdenu4 commented Sep 18, 2020 •

edited

Loading

dcdenu4 commented Oct 2, 2020 •

edited

Loading

davemfish commented Oct 2, 2020

dcdenu4 commented Oct 2, 2020

davemfish left a comment

davemfish Oct 5, 2020

dcdenu4 Oct 5, 2020

davemfish Oct 5, 2020

dcdenu4 Oct 5, 2020

davemfish Oct 5, 2020

davemfish Oct 5, 2020

dcdenu4 Oct 5, 2020

davemfish Oct 5, 2020

dcdenu4 Oct 5, 2020

dcdenu4 Oct 5, 2020

dcdenu4 commented Oct 5, 2020

phargogh commented Oct 5, 2020

dcdenu4 commented Oct 6, 2020 •

edited

Loading

davemfish left a comment

emlys left a comment

emlys Oct 7, 2020

dcdenu4 Oct 8, 2020

emlys Oct 7, 2020

dcdenu4 Oct 8, 2020

phargogh left a comment

phargogh Oct 9, 2020

phargogh Oct 9, 2020

phargogh commented Oct 9, 2020

phargogh commented Oct 9, 2020

phargogh commented Oct 9, 2020

InVEST 64-bit workflow using conda with Python 3.8 #314

InVEST 64-bit workflow using conda with Python 3.8 #314

Conversation

dcdenu4 commented Sep 18, 2020 • edited Loading

dcdenu4 commented Oct 2, 2020 • edited Loading

davemfish commented Oct 2, 2020

dcdenu4 commented Oct 2, 2020

davemfish 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

Choose a reason for hiding this comment

dcdenu4 commented Oct 5, 2020

phargogh commented Oct 5, 2020

dcdenu4 commented Oct 6, 2020 • edited Loading

davemfish left a comment

Choose a reason for hiding this comment

emlys 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

phargogh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phargogh commented Oct 9, 2020

phargogh commented Oct 9, 2020

phargogh commented Oct 9, 2020

dcdenu4 commented Sep 18, 2020 •

edited

Loading

dcdenu4 commented Oct 2, 2020 •

edited

Loading

dcdenu4 commented Oct 6, 2020 •

edited

Loading