-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
[python] Add setup requirements removing the need for a two-step install #1705
[python] Add setup requirements removing the need for a two-step install #1705
Conversation
Hi @john-bodley! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
de7f279
to
8c6a98e
Compare
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
8c6a98e
to
a094b72
Compare
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
a094b72
to
e81c58d
Compare
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
e81c58d
to
11fd11f
Compare
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Would love to see this merged! |
Bump. This issue plagues my Dockerfiles requiring multiple requirement.txt files to install. |
@quoimec In the mean time, on our end, I've temporary resolved it and we still keep one pip install $(grep -ivE '^fbprophet' requirements.txt)
pip install fbprophet So until this PR is merged, I just ignore |
Thanks for the PR. I know this is a big source of frustration but I'm not sure yet what the right fix is. I've looked into this particular approach in the past and it didn't look like it would work. There is a lot of discussion around this in #401. We tried to handle this with a pyproject.toml file (the way recommended by pip) but that didn't work because it builds the wheel in a separate build environment that may have different versions of pystan installed. We ran into an issue where pystan would build correctly in the system environment where the C++ toolchain was working properly, but failed in the build environment (see a description of it here: #401 (comment)). This made the install error out. As best as I understand it, the approach in this PR does a similar thing as using pyproject.toml: it sets up a build environment, uses The way I see it, the current behavior of having to pip install all of the dependencies before installing everything is annoying, but at least it consistently works. A situation where you can't install at all because the wheel won't build in the build environment is much worse, and that's what happened to us with pyproject.toml. So I'm pretty hesitant to do anything that might lead to that, and thus break the package for people. |
Oh, and I'm really sorry about the terribly slow response on this. |
There is at least one post, several GitHub issues (#1551, #401), and the Python documentation which recommends that you install
pystan
prior to installingfbprophet
as the build has an implicit dependency onpystan
,numpy
, etc.The issue with this two-step approach is it's problematic when using tools like
pip-tools
,pip-compile-multi
, etc. which purposely install/build all the requirements via a singlepip
command, and thoughpip
installs "dependencies before dependant", without explicit mention there is no guarantee that certain packages are installed prior to the building of thefbprophet
package resulting in errors of the form (truncated):A potentially viable solution (tested) is to explicitly define the requirements (specified in
requirements.txt
) assetup_requires
which invokes setuptools.dist.Distribution.fetch_build_eggs thus ensuring the requirements are installed prior to building the package and removing the need for the two-step approach.