Skip to content

Conversation

chriddyp
Copy link
Member

Closes https://github.com/plotly/dash-docs/issues/598

About

  • I am closing an issue

Description of changes

Right now, Dash Bio is incompatible with newer versions of Dash including that which we use in documentation. Ideally, component libraries have a loose dependency on Dash - the user should be able to upgrade and install Dash independently of the component package, and the component package shouldn't have to create a new release with every release of Dash.

Before merging

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-388 July 15, 2019 14:23 Inactive
@chriddyp chriddyp temporarily deployed to dash-bio-test-pr-388 July 15, 2019 14:23 Inactive
@chriddyp chriddyp requested review from shammamah-zz and mkcor July 15, 2019 14:23
@shammamah-zz
Copy link
Contributor

@chriddyp We have been holding off on doing this since the tests weren't passing: #367 (comment)

I'll try running the tests locally with dash==0.43.0, and if those pass then this should be good to go.

@chriddyp
Copy link
Member Author

chriddyp commented Jul 15, 2019

Could we lock the version for the tests (i.e. requirements.txt) but unlock the version for the end-users (i.e. setup.py)?

@shammamah-zz
Copy link
Contributor

Yes, that will work. I can take over the PR if you want!

@chriddyp
Copy link
Member Author

All yours, thanks @shammamah !

@shammamah-zz shammamah-zz had a problem deploying to dash-bio-test-pr-388 July 15, 2019 15:00 Failure
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-388 July 15, 2019 15:19 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-388 July 15, 2019 17:52 Inactive
@mkcor
Copy link
Contributor

mkcor commented Jul 15, 2019

I'll try running the tests locally with dash==0.43.0, and if those pass then this should be good to go.

This doesn't work and we've known it for weeks, if not months. We've been waiting for the testing framework refactoring.

I can link to the discussions where we report and detail all that for the n-th time, if needed.

@chriddyp
Copy link
Member Author

My main point is let’s not let our own testing refactoring prevent fixing this for our end users.

@shammamah-zz
Copy link
Contributor

@mkcor My mistake -- I thought the refactoring was completed in dash-1.0.0 (https://dash.plot.ly/testing).

@mkcor
Copy link
Contributor

mkcor commented Jul 15, 2019

My main point is let’s not let our own testing refactoring prevent fixing this for our end users.

Great, I'm all for it! If anything, I'm noticing that this decision could/should have been made earlier (we were blocked and that's not good, I just meant to remind that we knew we were blocked and why: #367 (comment)).

@shammamah don't forget to update the CircleCI config as well!

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-388 July 15, 2019 18:51 Inactive

# Additional packages needed to run the tests.
chromedriver-binary==2.45.0
dash==0.40.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create a double (at the moment, redundant) requirement, because of -r ../requirements.txt (top of the file). I suggest replacing the latter with all requirements except for dash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, revert d3993ea because it's already ensured through (top-level) requirements.txt). This way we have a clear and healthy use of setup.py vs requirements.txt files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2dd952e -- can't wait to unblock the situation...

@mkcor
Copy link
Contributor

mkcor commented Jul 16, 2019

@shammamah or @chriddyp please merge asap to resolve https://github.com/plotly/dash-docs/issues/598 and let Dash Bio's development follow smoothly that of the Dash ecosystem. Next up, we want to migrate to React 16!

@shammamah-zz shammamah-zz merged commit 761232d into master Jul 16, 2019
@shammamah-zz shammamah-zz deleted the chriddyp-patch-1 branch July 16, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants