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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

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

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
@rhugonnet
Copy link
Contributor

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
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
@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
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
Contributor

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

@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
@echo "To use: source ${VENV}/bin/activate; xdem -h"

#.PHONY: test
#test: ## run tests
Copy link
Member

Choose a reason for hiding this comment

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

I would keep the test command and add a test if GDAL is installed or not, for the test to work (if gdal is needed)

```bash
git clone https://github.com/GlacioHack/xdem.git
mamba env create -f xdem/dev-environment.yml
```

### With ``pip``

Copy link
Member

Choose a reason for hiding this comment

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

I would add documentation for this PR saying that GDAL is needed for development. Maybe in a section with make test ?

git clone https://github.com/GlacioHack/xdem.git
make install
```

After installing, you can check that everything is working by running the tests: `pytest`.
Copy link
Member

Choose a reason for hiding this comment

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

maybe separate a way with conda and a way with Makefile (as commented, to be discussed in next meeting for future way...)

@test -f .git/hooks/pre-commit || ${VENV}/bin/pre-commit install -t pre-commit
@test -f .git/hooks/pre-push || ${VENV}/bin/pre-commit install -t pre-push
@echo "XDEM installed in development mode in virtualenv ${VENV}"
@echo "To use: source ${VENV}/bin/activate; xdem -h"
Copy link
Member

Choose a reason for hiding this comment

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

the xdem cli doesn't exist maybe python3 -c "import xdem" or nothing for now

%(opt)s
%(test)s
Copy link
Member

Choose a reason for hiding this comment

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

Again, keep test dependency in dev, otherwise not really needed. And add check if gdal is installed on the computer, with documentation evolution.
When make install is done, I would expect a dev environment with pytest and dev package to use (pylint...).

setup.cfg Outdated
richdem
gdal
Copy link
Member

Choose a reason for hiding this comment

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

cf my global remark, It would be better to remove gdal if possible, in my opinon... in the future ?

@rhugonnet
Copy link
Contributor

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.

@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
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)

@test -f .git/hooks/pre-push || ${VENV}/bin/pre-commit install -t pre-push
@echo "Attempting to install GDAL..."
@make install-gdal
@echo "XDEM installed in development mode in virtualenv ${VENV}"
Copy link
Contributor

Choose a reason for hiding this comment

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

"xdem" in lower-case to match syntax above

@rhugonnet
Copy link
Contributor

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

@@ -72,8 +73,9 @@ doc =
myst-nb
numpydoc
dev =
pre-commit
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should move pre-commit to test dependencies (both here and in dev-environment.yml).
I've had some feedback that is unpractical to have to add it separately when doing the conda dev installation mamba env create -f dev-environment.yml. Would be more practical?
I don't know if there's a recommended practice for this.

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