Use bigger dtypes in routed models to expand number of supported pixels and implement docker container #1434
Conversation
…esn't like all the nested curly braces. RE:natcap#1431
This comes at a near-doubling of required stack memory (and some heap memory), but it should be OK for most users. RE:natcap#1431
phargogh
changed the title
phargogh
changed the title
…dels-have-int32-limitations
phargogh
changed the title
…431-sdr-and-probably-other-routed-models-have-int32-limitations Conflicts: HISTORY.rst
…nt32-limitations' of github.com:phargogh/invest into bugfix/1431-sdr-and-probably-other-routed-models-have-int32-limitations
This way pip will perform scm-based package installation like normal with whatever extra dependencies it needs. RE:natcap#1431
…dels-have-int32-limitations
…431-sdr-and-probably-other-routed-models-have-int32-limitations Conflicts: HISTORY.rst
…nt32-limitations' of github.com:phargogh/invest into bugfix/1431-sdr-and-probably-other-routed-models-have-int32-limitations
… version incompatibility that I encountered in github actions. RE:natcap#1431
When running on github actions, I found an error noting that `Hook
already exists: pre-push` (see below), which to me makes me think that
LFS already exists and has been configured. And I confirm that on GHA,
git-lfs is installed to version 3.4.0
```
git -C data/invest-test-data lfs install
Hook already exists: pre-push
#!/bin/sh
command -v git-lfs >/dev/null 2>&1 || { echo >&2 "\nThis repository is configured for Git LFS but 'git-lfs' was not found on your path. If you no longer wish to use Git LFS, remove this hook by deleting the 'pre-push' file in the hooks directory (set by 'core.hookspath'; usually '.git/hooks').\n"; exit 2; }
git lfs pre-push "$@"
To resolve this, either:
1: run `git lfs update --manual` for instructions on how to merge hooks.
2: run `git lfs update --force` to overwrite your hook.
```
A compiler will be needed to build from source. RE:natcap#1431
There was a problem hiding this comment.
Thanks @phargogh, I mostly left comments that was me talking to myself about what was taking place. The only thing I wanted to get your thoughts on was whether we should update the README with information about the docker container, or if a README should go in docker/ folder. Essentially, it would be nice to have some instruction on how to use the container for running InVEST. But that could also be it's own issue outside of this PR.
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| paths: |
There was a problem hiding this comment.
paths here is a filter indicating we only run when a file in one of these paths has changed?
There was a problem hiding this comment.
Yes, exactly. So the idea here was to only rebuild the container if there's something that will change the output of a model, like the source code, requirements, etc.
Makefile
Outdated
| @@ -196,15 +196,15 @@ $(GIT_UG_REPO_PATH): | |||
| $(GIT_SAMPLE_DATA_REPO_PATH): | $(DATA_DIR) | |||
| -git clone $(GIT_SAMPLE_DATA_REPO) $(GIT_SAMPLE_DATA_REPO_PATH) | |||
| git -C $(GIT_SAMPLE_DATA_REPO_PATH) fetch | |||
| git -C $(GIT_SAMPLE_DATA_REPO_PATH) lfs install | |||
| -git -C $(GIT_SAMPLE_DATA_REPO_PATH) lfs install | |||
There was a problem hiding this comment.
What's the argument for suppressing errors and continuing to install here? I'm wondering the same for above with the git clone command. Wouldn't we want to fail if this fails?
There was a problem hiding this comment.
Well, the git clone one, is pretty clear ... if the repo already exists at that path, git clone will fail and that's ok. It's more straightforward to do this than to handle the cross-platform if/else within the recipe.
For git lfs install, this one was failing (as far as I could tell) because git lfs is already installed and had installed some kind of hook that reinstallation would overwrite. But now that I'm looking for the failing builds, I'm less convinced that skipping on failure here is needed. Removing this in cd492aa
There was a problem hiding this comment.
if the repo already exists at that path, git clone will fail and that's ok. It's more straightforward to do this than to handle the cross-platform if/else within the recipe.
Ah, of course. That makes sense, thanks for the reminder.
| ARG INVEST_VERSION="main" | ||
| ARG INVEST_REPO="natcap/invest" |
There was a problem hiding this comment.
Cool, we're updating these placeholders from the GHA workflow call.
Updates stack (and a little heap) memory usage to expand the raster sizes our routing functions can support.
Fixes #1431
Fixes #1115
Checklist
- [ ] Updated the user's guide (if needed)- [ ] Tested the Workbench UI (if relevant)