-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
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 ☂️ |
Runs on Windows will only start passing once #210 is merged in (to clean up notebook names) |
There was a problem hiding this 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' |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍
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. |
@KasiaKoz I had to make another change for things to work on Windows: remove |
1ce9be0
to
a18b1f1
Compare
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.