search

symforce-org / symforce

Fast symbolic computation, code generation, and nonlinear optimization for robotics
https://symforce.org
Apache License 2.0
1.41k stars 145 forks source link

Python package metadata contains local paths #335

Open KOLANICH opened 1 year ago

KOLANICH commented 1 year ago

Describe the bug

Requires-Dist: symforce-sym @ file://localhost/PATH_TO_symforce_CLONED_REPO/gen/python

To Reproduce Steps to reproduce the behavior, e.g.:

  1. python3 -m build -nwx

Expected behavior No such things should be in wheels.

aaron-skydio commented 1 year ago

We have the SYMFORCE_REWRITE_LOCAL_DEPENDENCIES environment variable to control this - when building wheels to distribute, the intention is to set that environment variable, and distribute wheels for symforce-sym and skymarshal separately. If that doesn't work for some reason we should fix that.

Agree this isn't ideal, the reason it's set up like this is so that pip install . from the SymForce directory "just works".

I'm curious if there's a specific issue this is causing, or if there's a specific proposal to change this in a way that both supports building standalone wheels for these things and one-line from-source installs

KOLANICH commented 1 year ago

Thanks for the info and I'm sorry for the inconvenience of the trivial questions, I really haven't known that for building a wheel properly I need to set some environment variables. I close this issue for now, because I currently have no plans to rebuild the wheel (it takes a lot of time and a lot of memory, and some manual tweaks (like digging into files and disabling tests, since building them takes even more memory)) in order to test if setting the var solves the issue.

aaron-skydio commented 1 year ago

We aren't supposed to be building tests, examples, or benchmarks when building wheels (benchmarks don't build by default, which is why they aren't explicitly disabled there, although it wouldn't be a bad idea to explicitly disable them also): https://github.com/symforce-org/symforce/blob/c3d2f3c97d77e8d9015276cd9ae7b080df646fc3/setup.py#L114-L119

If we are, that's a bug that we should also fix