-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix ngen image for building t-route #474
Fix ngen image for building t-route #474
Conversation
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.
Only needs a minor change to add support for arm. Looks good otherwise!
Aside, I don't know if we want to address this here or not, but on line 13 of the Dockerfile, ARG NGEN_BUILD_CONFIG_TYPE="Release"
is not causing ngen
, ngen-serial
, or ngen-parallel
to be built in Release mode. See the rocky_build_ngen
build stage line ~565.
cat ${WORKDIR}/t-route-requirements.txt | grep -i pyarrow >> first_reqs.txt ; \ | ||
pip3 install --no-cache-dir -r first_reqs.txt ; \ | ||
rm first_reqs.txt ; \ | ||
# Then install a few more things, including a particular blosc2 version and an adjusted build of tables \ |
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.
Looks like
fiona
is the culprit.geopandas
depends onfiona
andfiona
by default will install frompip
with a pre-builtlibgdal
, if it can. It seems like the issue isfiona
does not have a distribution with pre-buildlibgdal
for the particular target.Fiona has several extension modules which link against libgdal. This complicates installation. Binary distributions (wheels) containing libgdal and its own dependencies are available from the Python Package Index and can be installed using pip.
# Then install a few more things, including a particular blosc2 version and an adjusted build of tables \ | |
# Then install a few more things, including a particular blosc2 version and an adjusted build of tables \ | |
dnf install -y gdal-devel ; \ |
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 fixed this differently, by including gdal-devel
in the main step where the list of packages to be installed via dnf
is defined. It'll be installed now in an ancestor build stage of the one the suggestion would have been in.
Fixing application of value intended for CMAKE_BUILD_TYPE in various ngen repo code build gen steps within ngen-based images.
Add gdal-devel as image package dependency to ensure proper ARM support, needed due to an issue with fiona and libgdal on ARM.
I addressed the |
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.
Looks good to me! Thanks for making those changes. Merge at will.
Modifying image to get t-route built successfully, so that it is possible to run a NextGen execution that includes t-route routing.
Partially (but not completely) addresses #472; i.e., the image will successfully run serial
ngen
execution with at least some valid configurations of t-route. Remaining problematic cases appear to be not directly related to the worker image itself.