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

Makefile initial installation #630

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

vschaffn
Copy link
Contributor

@vschaffn vschaffn commented Oct 31, 2024

Resolves #582.

Description

This PR introduces the initial Makefile for the xdem project, aimed at streamlining the installation process for developers on Linux systems. By implementing a Makefile, we provide a simple command-line interface for common setup tasks, enhancing the developer experience and reducing the potential for errors during installation.

Key Changes:

  1. Makefile Structure: The Makefile has been created based on the existing structure of the demcompare project.

  2. Targets Implemented:

    • help: Displays help information about available targets.
    • venv: Creates a virtual environment in the specified directory if it does not exist.
    • install-gdal: Installs the version of GDAL corresponding to that of the system.
    • install: Installs the xdem package in editable mode along with necessary dependencies and sets up pre-commit hooks for Git.
    • clean: A comprehensive clean target to remove build artifacts, virtual environment, and pre-commit hooks.
    • test: Run pytest (if GDAL is in the virtual environment).
  3. Update dependencies:
    The dependencies used in each sections (required, optionnal, test, doc) have been updated to match with the projet.

Documentation

The documentation https://xdem.readthedocs.io/en/stable/how_to_install.html, README.md and CONTRIBUTING.md have been updated with the make install command for contributors.

Constraints

  • GDAL is needed on the system to be installed on the virtual environment and run the tests.
  • For now richdem is not stable enough to build a wheel on python > 3.10. For the moment, the make install command only works on python 3.10 and fails if this version is not installed on the system.

@vschaffn vschaffn changed the title 582 makefile installations Makefile initial installation Oct 31, 2024
@vschaffn vschaffn force-pushed the 582-makefile_installations branch from 523f1b5 to 88ebde1 Compare October 31, 2024 16:11
@rhugonnet
Copy link
Member

rhugonnet commented Oct 31, 2024

I am not very familiar with Makefile builds (@adehecq is much more). If my understanding is correct, this would replace/improve the current developer installation via mamba + dev-environment.yml shown here: https://xdem.readthedocs.io/en/stable/how_to_install.html#installing-for-contributors?

For us (even before xDEM, and on Python 2), using the conda-forge channel (with conda or mamba) to solve for GDAL + anything else was always the successful recipe. Is that possible within a Makefile build?
But even through conda-forge, GDAL + RichDEM did occasionally break in CI from time to time in the past 4 years 😅 (once or twice, for instance #376; but has been passing on all OS and Python versions recently).

We could also coordinate with Richard Barnes to solve the issue.

@vschaffn vschaffn force-pushed the 582-makefile_installations branch from 88ebde1 to f6fc3b9 Compare November 5, 2024 13:28
@vschaffn
Copy link
Contributor Author

vschaffn commented Nov 5, 2024

@rhugonnet the aim of this makefile is to install easily xdem in develoment mode with pip, so it would not replace/improve mamba installation but an alternative without using conda

@vschaffn vschaffn force-pushed the 582-makefile_installations branch 4 times, most recently from 8cf6306 to 885e20b Compare November 12, 2024 09:28
@vschaffn vschaffn force-pushed the 582-makefile_installations branch from 885e20b to 977358d Compare November 14, 2024 16:45
@adebardo
Copy link

Hello !

  • The 'make test' command doesn't work for me. If it's because of GDAL, the best thing to do is delete the line in the Makefile for the moment.

@adebardo
Copy link

@duboise-cnes Could you give us your opinion on the dependances issues?

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@adebardo
Copy link

@rhugonnet Would you agree to update the readme by indicating that a pip install is viable?

@adebardo
Copy link

adebardo commented Nov 18, 2024

We also should update documentation : https://xdem.readthedocs.io/en/stable/how_to_install.html with the makefile option for contributors

@vschaffn vschaffn force-pushed the 582-makefile_installations branch from 977358d to 10df937 Compare November 18, 2024 17:16
@vschaffn
Copy link
Contributor Author

@adebardo For now I have commented the make test. The documentation https://xdem.readthedocs.io/en/stable/how_to_install.html has been updated with the make install.

@rhugonnet
Copy link
Member

Yes, let's add the pip option to the README and the makefile option to the "How to install" page!

@vschaffn vschaffn force-pushed the 582-makefile_installations branch from 10df937 to 4a85f06 Compare November 19, 2024 09:09
@duboise-cnes
Copy link
Member

I am not very familiar with Makefile builds (@adehecq is much more). If my understanding is correct, this would replace/improve the current developer installation via mamba + dev-environment.yml shown here: https://xdem.readthedocs.io/en/stable/how_to_install.html#installing-for-contributors?

For us (even before xDEM, and on Python 2), using the conda-forge channel (with conda or mamba) to solve for GDAL + anything else was always the successful recipe. Is that possible within a Makefile build? But even through conda-forge, GDAL + RichDEM did occasionally break in CI from time to time in the past 4 years 😅 (once or twice, for instance #376; but has been passing on all OS and Python versions recently).

We could also coordinate with Richard Barnes to solve the issue.

Makefile is not really "needed" absolutely but it is common tool in linux development and a simple wrapper for development practise.
I think for support for other developers, the make clean is really great to go back to a clean state for instance. I don't know how to do that with conda for instance. It also suppose to be on a python3 pip pure install and therefore is more general than conda environment to handle possible dependencies problems. Anyway, in our internal chains, we don't use conda and having a developer environment way without it ensure compatibility. And we have generalized the makefile to organize common way to understand a dev python repository. Just clone and do a make help and you have the supported dev recipes of the project...

I understand that conda ease the way to have GDAL (even if you are not root on your pc). But in our experience, we have tried to remove GDAL direct dependency because of difficulty of maintenance and use it through rasterio directly in python3 or other way.

So I would propose two steps:

  • finish this PR adding dependency of GDAL for the make install in documentation and in Makefile doing a test for GDAL presence and crash if not present
  • second step, discuss possibilities in next meeting for getting rid of gdal direct dependency if ok or possible. (use through rasterio, have reference generated outside the code). I think even for contributors that it would be better not having direct GDAL dependency for maintenance in XDEM. My opinon, but to be exchanged. For developers, it is not so difficult to get GDAL (we have it on the cnes cluster) and there is anyway the conda approach in parallel.

As for richdem, if not so maintained for different python, can the dependency be removed, included, updated as discussed (if i remember well) ? Again, maybe in another PR, not sure

I have several comments on the code line by line in following comments.

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@rhugonnet
Copy link
Member

Thanks @duboise-cnes, it's much clearer!

For GDAL/RichDEM: They are not dependencies for the users, but they are for developers (only used in tests).

They are quite important as they verify all types of terrain attributes (slope, aspect, TPI, TRI, etc), custom reprojection in the same CRS (to avoid sub-pixel errors of Rasterio that haven't been fixed in more than a year), etc. Same for RichDEM to check curvatures.

I'd advocate for keeping them. But if we want to remove them, that's doable too 🙂.
We could generate output files from GDAL/RichDEM (slopes, curvatures, etc) in the xdem-data repo instead, then retrieve those and continue testing in xdem even without the dependencies.

@vschaffn vschaffn force-pushed the 582-makefile_installations branch 2 times, most recently from 8dbb2b9 to f9f0bf9 Compare November 21, 2024 09:40
@duboise-cnes
Copy link
Member

Thanks @duboise-cnes, it's much clearer!

For GDAL/RichDEM: They are not dependencies for the users, but they are for developers (only used in tests).

They are quite important as they verify all types of terrain attributes (slope, aspect, TPI, TRI, etc), custom reprojection in the same CRS (to avoid sub-pixel errors of Rasterio that haven't been fixed in more than a year), etc. Same for RichDEM to check curvatures.

I'd advocate for keeping them. But if we want to remove them, that's doable too 🙂. We could generate output files from GDAL/RichDEM (slopes, curvatures, etc) in the xdem-data repo instead, then retrieve those and continue testing in xdem even without the dependencies.

It was clear for me that it was only for developers. But even for developers, the dependency can be complex.
I completely agree on the need of reference, stable enough. but can be difficult also with gdal as you said previously. But maybe your proposition is a good one, generate the data and depend on it, and not on the tool that have generated the data.
I would advocate a development version without gdal, but of course debatable and we could work with the choice also.
Maybe discuss it in next meeting point ?

Richdem seems not so well maintained and with the first move on the licence, I would advocate also to not have the development installation blocked by it. Same approach can be taken.

@vschaffn vschaffn force-pushed the 582-makefile_installations branch from f9f0bf9 to f26b986 Compare November 22, 2024 15:41
@vschaffn
Copy link
Contributor Author

@adebardo @rhugonnet @duboise-cnes The PR has been updated, I have found a way to solve the GDAL configuration problem. Now the makefile works if the user has GDAL on the system (if not the makefile gives instructions on how to install it). We still have constraints on richdem with python > 3.10, so for the moment the makefile is only configured for python3.10.
A check has been added to make test so that it does not run if GDAL is not in the virtual environment.
I have kept "To use: source ${VENV}/bin/activate; xdem -h" to anticipate the CLI (#629)

Makefile Outdated Show resolved Hide resolved
@rhugonnet
Copy link
Member

@vschaffn Perfect for the solve! Not much to comment on my side, looks great :)

setup.cfg Outdated Show resolved Hide resolved
@vschaffn vschaffn force-pushed the 582-makefile_installations branch from f26b986 to 8235de2 Compare November 26, 2024 10:09
@adebardo adebardo merged commit e0afc9d into GlacioHack:main Nov 27, 2024
19 checks passed
@vschaffn vschaffn deleted the 582-makefile_installations branch November 27, 2024 11:08
@rhugonnet
Copy link
Member

@vschaffn @adebardo @duboise-cnes I realized today that RichDEM is simply not distributed on PyPI since 6 years: https://pypi.org/project/richdem/. It's only regularly updated on conda: https://anaconda.org/conda-forge/richdem. So the only option to make it work in the Makefile would likely be to build it from source, as recent releases are available on GitHub: https://github.com/r-barnes/richdem/releases. Some discuss this here: r-barnes/richdem#65.

Otherwise the removal we talked about to xdem-data, which is fine by me! 🙂

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.

[POC] Makefile installations
4 participants