search

giotto-ai / giotto-tda

A high-performance topological machine learning toolbox in Python
https://giotto-ai.github.io/gtda-docs
Other
847 stars 173 forks source link

pybind11 and cmake as build dependencies in pyproject.toml #314

Open rth opened 4 years ago

rth commented 4 years ago

Both pybind11 and cmake are on PyPi and conda. Can we not specify them as regular dependencies instead of manually installing in setup.py which leads to issues like https://github.com/giotto-ai/giotto-tda/issues/308 ?

In general pyproject.toml might be a solution for specifying build time dependencies. See https://www.python.org/dev/peps/pep-0518/ for more details.

cc @ulupo @MonkeyBreaker @gtauzin

MonkeyBreaker commented 4 years ago

I don't think that adding cmake as dependencies could be a problem. The issue about pybind11 is that we only need the headerd (c++) files, I didn't check what's downloaded when using pip install pybind11.

About issue #308 Even if we add these dependencies, we have some submodules in gtda/externals/ I don't know how we could download them. Do you have any suggestions ?

rth commented 4 years ago

About issue #308 Even if we add these dependencies, we have some submodules in gtda/externals/ I don't know how we could download them.

Indeed. One way could be to make sure submodule contents get included in the .tar.gz then there there is not need to run git submodule update. Maybe this is already happening now, not sure.

In any case I think that command should be run explicitly during the development installation process instead of automatically in setup.py.

MonkeyBreaker commented 4 years ago

In any case I think that command should be run explicitly during the development installation process instead of automatically in setup.py.

In this case, the developer must do git submodule update before trying to use it. Is it a big requirement for the developer ? I mean, currently he needs to:

pip install -e .

Then he will need to do:

git submodule update --init --recursive # But only once
pip install -e .

I don't think is to much of a burden for the developers, but previous developers would need a notification about this change, I don't know what do you think about it @ulupo ?

ulupo commented 4 years ago

but previous developers would need a notification about this change, I don't know what do you think about it @ulupo

This one is thorny...