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

Fix ngen image for building t-route #474

Merged

Conversation

robertbartel
Copy link
Contributor

@robertbartel robertbartel commented Jan 4, 2024

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.

@robertbartel robertbartel added bug Something isn't working maas MaaS Workstream Blocks Another issue is blocked until this is resolved labels Jan 4, 2024
@robertbartel robertbartel marked this pull request as ready for review January 22, 2024 14:27
Copy link
Member

@aaraney aaraney left a 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 \
Copy link
Member

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 on fiona and fiona by default will install from pip with a pre-built libgdal, if it can. It seems like the issue is fiona does not have a distribution with pre-build libgdal 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.

source

Suggested change
# 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 ; \

Copy link
Contributor Author

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.
@robertbartel
Copy link
Contributor Author

I addressed the gdal-devel item and fixed things with the value intended for CMAKE_BUILD_TYPE. Should be ready for re-review now.

Copy link
Member

@aaraney aaraney left a 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.

@robertbartel robertbartel merged commit fde79d5 into NOAA-OWP:master Jan 24, 2024
0 of 4 checks passed
@robertbartel robertbartel deleted the f/ngen_image_troute_build_fix/main branch January 24, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Another issue is blocked until this is resolved bug Something isn't working maas MaaS Workstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants