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

Update install to rely on mamba / conda envs only #214

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

brynpickering
Copy link
Contributor

Somewhat supersedes #212, but is in line with the steps that #188 was broken into.

@KasiaKoz the CBC issue is solved by explictly including it in the creation of the mamba environment. I'm chasing up the CBC devs to get the windows version sorted but until then it has to be added explicitly in the install instruction (no different to now tbh) and CI tests on windows will (once I push the fix) skip tests that require CBC.

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@brynpickering
Copy link
Contributor Author

Runs on Windows will only start passing once #210 is merged in (to clean up notebook names)

Copy link
Contributor

@mfitz mfitz left a comment

Choose a reason for hiding this comment

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

Good stuff.

Out of interest - has the Docker image changed in size? I think purging the Node install should be worth something like -50 to -100 megs, but the change in base image is probably the biggest influence on image size.


on:
schedule:
- cron: '37 14 * * 1-5'
- cron: '23 14 * * 1-5'
Copy link
Contributor

Choose a reason for hiding this comment

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

I INSIST ON RUNNING THIS ON THE 37th MINUTE!!! 🤣

(Actually, thinking about it, 37 actually does carry a very small advantage over 23, in that it makes clear that this is the minute, rather than the hour, but that's admittedly a marginal gain)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what, I did think it was hours 😅 maybe I should change it back to 37...

@@ -19,3 +21,5 @@
from genet.schedule_elements import Route, Schedule, Service, Stop
from genet.use.road_pricing import Toll
from genet.utils import elevation, google_directions, graph_operations

pyproj.network.set_network_enabled(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It stops it from trying to connect to the network to download transformation gridfiles on-the-fly (see #213). If it does try, there is a bug with handling self-signed SSL certificates that will cause an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't needed before because the PROJ environment variable associated with this is set to OFF when downloading from homebrew/apt, but is ON when downloading from conda-forge (because in the former it automatically downloads the proj-data dataset too while it isn't included in the latter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See OSGeo/PROJ#3977 and pyproj4/pyproj#1233 for conversations I've initiated on it in upstream projects

@@ -131,6 +133,8 @@ def test_service():
def test_snapping_pt_route_results_in_all_stops_with_link_references_and_routes_between_them(
test_network, test_spatialtree
):
if not shutil.which("cbc"):
pytest.skip("CBC solver not installed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea 👍

@brynpickering
Copy link
Contributor Author

RE image size, I think it should reduce. However, I can't verify this as I've got local comparisons based on different container platforms (it can't find conda packages on Linux ARM so I have forced this branch to run in an AMD container [how does that even work?!]). Will wait to have an updated CodeBuild on AWS to see the comparison on there.

.github/workflows/pr-ci.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
@brynpickering
Copy link
Contributor Author

@mfitz RE image size. It is smaller than the old image (incl. NodeJS) by about 5% but larger than the image in #212 by about 6%.

@brynpickering
Copy link
Contributor Author

brynpickering commented Dec 20, 2023

@KasiaKoz I had to make another change for things to work on Windows: remove : from filenames. This affects one of GeNet's outputs: vph_all_modes_within_{h - 1}:30-{h}:30.geojson. I opted for _ to replace :.

@brynpickering brynpickering merged commit a7e484f into main Jan 8, 2024
10 checks passed
@brynpickering brynpickering deleted the mamba-package-install branch January 8, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants