-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
Add optional download requirements for dev and plotly express #4644
base: master
Are you sure you want to change the base?
Conversation
We can plausibly drop the note about graph objects but basically I want some sort of explanation of why |
We shouldn't recommend If we want to have a one-liner "install everything" command that should be a change made inside Plotly.py. |
I was thinking of adding:
as an option which goes with usual pip patterns for optional dependencies. If we went that route I would expect that we drop support for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed from the PR title that this was going to improve the install-time or runtime error messages for "you need Pandas" but there seem to be other changes as well? I trust you...
@@ -137,17 +137,17 @@ conda activate plotly-dev | |||
|
|||
### Install requirements - (Non-Windows) | |||
```bash | |||
(plotly_dev) $ pip install -r packages/python/plotly/requirements.txt | |||
(plotly_dev) $ pip install -r packages/python/plotly/optional-requirements.txt | |||
(plotly_dev) $ pip install -r packages/python/plotly/requires-install.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these related to the pandas warning change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is related to the updates to requirements files.
👍 on breaking the PR - thank you |
IMO the naming should be I'm in favor of this change, but let's not merge it right away. This PR changes a number of lines in |
83fb8c8
to
e2bcd8b
Compare
@gvwilson on further thought I think it makes sense to keep these changes in one PR. The text in the warning message is directly related to the installation recommendations that are also added in this PR -- I've updated the title & description to make the primary change more clear. |
👍 |
Co-authored-by: Alex Johnson <[email protected]>
@emilykl is this PR good to go with the latest changes? |
@marthacryan could you take a look at this? |
See also #4790 |
@ndrezn So is |
@marthacryan |
Oh interesting. What is the benefit here over just using |
The benefit is we recommend a specific version number for compatibility. I don't think we have issues with different versions of Pandas but this makes it easy to pin a version of Pandas for use with Plotly if we ever encounter issues. In fact we're doing that here, where we install the latest pandas for new versions of Python but force older pandas for older Python. People in the original issue were having funky errors caused by this incompatibility: #2279 (comment) |
Oh ok that makes sense! Maybe for documenting the usage of that we could make that more clear? |
Closes #2279
This PR adds the ability to install Plotly with:
which bundles
pandas
in the install, required byplotly.express
and:
which installs dev requirements for the package.
This also revises the warning when using
plotly.express
ifpandas
is not installed.Open to ideas on better wording.