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

Bokeh Requirement in Correct File #687

Merged
merged 1 commit into from Oct 5, 2017
Merged

Bokeh Requirement in Correct File #687

merged 1 commit into from Oct 5, 2017

Conversation

brittainhard
Copy link
Contributor

No description provided.

@hdoupe hdoupe merged commit 470a98c into master Oct 5, 2017
@hdoupe
Copy link
Collaborator

hdoupe commented Oct 5, 2017

After updating Bokeh to the most recent version 0.12.9, the Cost-of-Capital calculator loads but displays an error message. I tested this on my local machine and CCC worked fine using Bokeh 0.12.7, but after upgrading to 0.12.9, I received an error message:
btax_localrun_1

One option is to try pinning Bokeh to 0.12.7. Admittedly, this is a sub-optimal solution. However, it would be a top priority to change the plot to move Bokeh up to the most up-to-date version before the next release.

@MattHJensen @martinholmer @brittainhard @hayleefay

@martinholmer
Copy link
Contributor

@hdoupe, See the discussion about bokeh instability in pull request #642.
The bokeh package seems pretty flaky in the last few months with major regressions when new versions are introduced. Why did we adopt bokeh when there is a much more stable and full-featured graphics options?

@MattHJensen @GoFroggyRun @brittainhard

@hayleefay
Copy link
Contributor

hayleefay commented Oct 5, 2017

@hdoupe Most likely updating the version in these link and script tags for the Bokeh widgets will solve the problem. But, in the long run this will keep happening if we do not pin a specific version, since those CDNs are pinned to a specific version. I tried to find a way around that but the updating CDN tags don't seem to be available for the widgets.

And, as @martinholmer pointed out, there have been some breaking changes in the latest releases that may still be of concern without a pinned version.

@hayleefay
Copy link
Contributor

@hdoupe And of course as soon as I say that I figure out what to do. Should I put up a pull request? Or I can just tell you where to add two lines of code if you like.

@hdoupe
Copy link
Collaborator

hdoupe commented Oct 5, 2017

@hayleefay Great, please open a pull request.

@brittainhard
Copy link
Contributor Author

@hayleefay @hdoupe I think the best thing to do is pin the bokeh version. Bokeh is still in activate development and changes frequently.

@hayleefay
Copy link
Contributor

@brittainhard @hdoupe The PR I just submitted allows the visualization to work on 0.12.9 and doesn't expressly need a pinned version anymore, but I agree with the decision to pin a version due to development concerns.

@hdoupe
Copy link
Collaborator

hdoupe commented Oct 6, 2017

@brittainhard @hayleefay I agree that we should pin it, too. Throwing in a quick fix like this sort of short-cuts the release process. I'm new to managing this process, and I want to make sure that we establish good habits early. So, let's pin the bokeh to 0.12.7 and slate PR #688 for the next release.

@hayleefay thanks for the quick response on this issue.

@hdoupe hdoupe mentioned this pull request Oct 6, 2017
@martinholmer
Copy link
Contributor

@hdoupe said:

I agree that we should pin it, too. Throwing in a quick fix like this sort of short-cuts the release process. I'm new to managing this process, and I want to make sure that we establish good habits early. So, let's pin the bokeh to 0.12.7

This is an OK approach only if it is a short-term fix. The versions should ideally be package>=x.y.z not
package==x.y.z.

I thought the pull request submitted by @hayleefay made the charts work with with both 0.12.7 and 0.12.9.
Is that not true? What about bokeh 0.12.8? Do the charts work with that version?

@hayleefay
Copy link
Contributor

@martinholmer, my latest PR does work with both 0.12.7 and 0.12.9. Barring breaking changes in future versions, a version of bokeh>=0.12.7 will be just fine. Oddly enough there does not appear to be a 0.12.8.

@martinholmer
Copy link
Contributor

martinholmer commented Oct 6, 2017

@hayleefay said in #687:

my latest PR does work with both 0.12.7 and 0.12.9. Barring breaking changes in future versions, a version of bokeh>=0.12.7 will be just fine. Oddly enough there does not appear to be a 0.12.8.

OK, thanks for the helpful information. So, why isn't PolicyBrain using bokeh>=0.12.7?
I don't understand what the need for the == pinning.

@brittainhard and @hdoupe, can you please explain? Are you thinking there are elements other than the charts by @hayleefay that will fail when using bokeh 0.12.9?

@MattHJensen

@MattHJensen
Copy link
Contributor

@martinholmer said:

This is an OK approach only if it is a short-term fix. The versions should ideally be package>=x.y.z not package==x.y.z.

@martinholmer, could you explain why you believe >= is preferable to ==? And what are the conditions that need to be met for a project to safely handle >= pinning?

@hdoupe hdoupe mentioned this pull request Nov 8, 2017
3 tasks
@martinholmer martinholmer deleted the bhard/bokeh_version branch December 1, 2017 22:58
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.

None yet

5 participants