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

See if we can improve installation flow for editable vs non-editable mode. #91

Open
tlpss opened this issue Sep 25, 2023 · 11 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@tlpss
Copy link
Contributor

tlpss commented Sep 25, 2023

improve dependencies installation flow.

atm, the internal dependencies are installed using the [external] tag during pip install, to avoid overwriting editable installs with non-editable installs.

It would be better to fix this in code with the setup.py file. We detect if the setup.py was called in editable mode, we simply remove the local dependencies (these should have been installed manually in that case).

This would make this transparent for the user, which is beneficial imo.

Even better would be to auto-install the dependencies in editable mode as well, but I haven't managed to get that working in the past.

cf. https://github.com/open-mmlab/mmsegmentation/blob/main/setup.py

@tlpss tlpss added the enhancement New feature or request label Sep 25, 2023
@tlpss
Copy link
Contributor Author

tlpss commented Dec 5, 2023

@Victorlouisdg ^

@tlpss
Copy link
Contributor Author

tlpss commented Dec 5, 2023

using this we should be able to catch if install is in editable mode or not. If in editable mode, don't install airo-mono dependencies because we want to have those editable as well and they would be overwritten by a non-editable install afaik. If not, we add the other dependencies (not sure how this would work for pypi releases though, because it installs the packages using local source code).

@Victorlouisdg
Copy link
Contributor

To summarize:

Proposed developer installation

we want the installation for developers (in an existing conda env) to look like this:

git clone git@github.com:airo-ugent/airo-mono.git
cd airo-mono
pip install -e airo-camera-toolkit -e airo-dataset-tools -e airo-robots -e airo-spatial-albegra -e airo-teleop -e airo-typing

Ideal user installation

For users, the ideal installation would be with pypi, e.g:

pip install airo-camera-toolkit airo-dataset-tools airo-robots airo-spatial-albegra airo-teleop airo-typing

Propsed user installation

But for now we don't have releases, so it would be e.g.:

pip install 'airo-robots @ git+https://github.com/airo-ugent/airo-mono@main#subdirectory=airo-robots'
pip install 'airo-camera-toolkit @ git+https://github.com/airo-ugent/airo-mono@main#subdirectory=airo-camera-toolkit'
...

Where the [external] is not needed anyone, because if we detect the install is non-editable, we add the dependencies to install_requires like so:

 f"airo_typing @ file://localhost/{root_folder}/airo-typing",

@tlpss
Copy link
Contributor Author

tlpss commented Dec 5, 2023

yes but to create releases, I think we also need to avoid these
f"airo_typing @ file://localhost/{root_folder}/airo-typing",

because I think that will fail if you donwload a wheel from pypi

@Victorlouisdg
Copy link
Contributor

I was wondering whether creating pypi releases might solve the problem with editable installs as well? Say we run

pip install -e airo-spatial-albegra
pip install -e airo-robots

With this in the airo-robots setup.py:

install_requires=[..., "airo-spatial-algebra >= 0.2.0"]

Then I believe pip might detect that this is satisfied and might not uninstall the already existing editable version?

If this is the case, shouldn't we just pursue releases on pypi instead of customizing our setup.py (further)?

@tlpss
Copy link
Contributor Author

tlpss commented Dec 8, 2023

hm I'm not sure, I had the feeling that pip overruled an editable install, but we would have to test. Maybe we should try it out on a separate branch? we could modify the setup.py of a few packages, create wheels, publish them on pypi and see how it goes

@tlpss
Copy link
Contributor Author

tlpss commented Jan 2, 2024

Spent some time thinking/trying around. Below is a summary

current situation
What we now have are three ways to install packages from the mono repo:

  • editable, local install through pip install -e for each script
  • non-editable, local install through the use of direct links in the setup.py files
  • non-editable, remote install by using pip's ability to install the setup.py files from git repos (bundled in this script )

The non-editable installs currently use an addional extra_requires arg in the setup.py since pip overwrites editable installs when installing from a direct link, which is confusing.

Furthermore, the use of direct links (the file @ specifiers used in the setup.py for the internal dependencies), creates some issues when updating(was encountered during the IRM course), see here.

Actually I have slightly abused these direct links with file paths to be able to specify internal dependencies without building releases for the packages. Next to the aforementioned issue with the stale wheels, this will also break wheels that are built in regular fashion using setuptools. So I think we need to abandon them in favor of a proper way to deal with our local dependencies.

proposed solution

  • setup.py now lists both internal and external dependencies, as usual.

for local, editable installs, you install them in the proper order or in a single line using pip install -e.

For non-editable installs, you use wheels. These wheels can be released on PyPI or in another place(such as locally on your desktop), in which case you use pip install --find-link (see here).

We provide a script and/or CI workflow to build all wheels (which can then be uploaded to github/PyPI)

@Victorlouisdg what do you think? (note I skip version management in this analysis, though both are slightly related as they are typically handled by the same tools)

Alternatives could be:

  • use manager that is more powerful, such as poetry and can handle internal path dependencies vs external wheel dependencies.
  • use dynamic setup.py code to detect if we are in editable mode, and to filter out the internal dependencies if that is the case. This would still not allow to release packages though.

@Victorlouisdg
Copy link
Contributor

The proposed solutions sounds great, less customization and basically no regression in ease of use (expect for the non-editable local installations, which should be rarely needed).

We provide a script and/or CI workflow to build all wheels

Ideally this be should be something close to python -m build.

To install all packages in editable mode, I guess the command would be this?

pip install -e airo-typing -e airo-spatial-algebra -e airo-dataset-tools -e airo-robots -e airo-teleop -e airo-camera-toolkit

@tlpss
Copy link
Contributor Author

tlpss commented Jan 2, 2024

We provide a script and/or CI workflow to build all wheels

Ideally this be should be something close to python -m build.

Indeed, and then do this for each package.

To install all packages in editable mode, I guess the command would be this?

pip install -e airo-typing -e airo-spatial-algebra -e airo-dataset-tools -e airo-robots -e airo-teleop -e airo-camera-toolkit

Indeed, though I think you only need to pass the -e flag once.

Directly installling from github will then be no longer supported and we will have to modify the current install script. Only thing to keep in mind is that the direct install allowed to install from any commit ID, whereas this will now only work with a git clone and editable install (though this is what happens behind the scenes anyways)

@tlpss tlpss self-assigned this Jan 3, 2024
@tlpss
Copy link
Contributor Author

tlpss commented Jan 3, 2024

also need to change the testing workflows, now each package is installed isolated, but this will no longer work unless wheels are built each time. For now I think it's easier to simply install all packages first from local files and to only then run the tests. Alternative is to first build distributions for each package once and to then start running tests for each package and its dependencies.

tlpss added a commit that referenced this issue Jan 12, 2024
This PR changes how internal dependencies are handled.

- List internal dependencies as regular dependencies.
- Modify the install commands to install them all at once (or in the right order)
- Fix issues with 'direct links' and filepaths to list the internal dependencies by list them as regular dependencies.
cf #91 

* initial setup

* dump

* test

* dump

* backup

* scheme implemented

* versioning in place

* first version

* slight change in versioning: main is kept at last released version

* make build version strings sorted along date-time

* update docs

* list internal dependencies as regular dependencies instead of using direct links with filepaths

also exclude test/ folder from setuptools packages

* install all packages during CI actions  to avoid issues with internal dependencies

* fix missing traling slashes for packages to point to locations

* update installation instructions and install script

* update changelog
@Victorlouisdg
Copy link
Contributor

The current pattern of installing with direct references is not allowed if you want to publish your package that depends on airo-mono on PyPi:

Public index servers SHOULD NOT allow the use of direct references in uploaded distributions. Direct references are intended as a tool for software integrators rather than publishers.
https://peps.python.org/pep-0440/#direct-references

This will be fixed once we publish the airo-mono packages on PyPi.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants