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

Remove bokeh plot and replace with simple matplotlib plot #767

Merged
merged 4 commits into from
May 6, 2021

Conversation

romanlutz
Copy link
Member

Our doc builds are taking a while, so it seemed like a good idea to replace the custom bokeh plot with a simpler matplotlib plot just like in #766 . Sadly, we are locked into all dependencies we ever had unless we're willing to go into older versions of Fairlearn, remove them from there, and then move the tag. Still, this simpler plotting code seemed like a step in the right direction.

Roman Lutz added 4 commits May 3, 2021 20:53
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
Signed-off-by: Roman Lutz <rolutz@microsoft.com>
@romanlutz romanlutz added dependencies Pull requests that update a dependency file CI Continues Integration and build related issues labels May 4, 2021
@romanlutz romanlutz requested a review from riedgar-ms May 4, 2021 06:08
@adrinjalali
Copy link
Member

Sadly, we are locked into all dependencies we ever had unless we're willing to go into older versions of Fairlearn, remove them from there, and then move the tag.

I'm not sure what you mean here @romanlutz

@romanlutz
Copy link
Member Author

Sadly, we are locked into all dependencies we ever had unless we're willing to go into older versions of Fairlearn, remove them from there, and then move the tag.

I'm not sure what you mean here @romanlutz

Let me explain what I initially thought, and then my correction to what I wrote:
@riedgar-ms encountered the same issue before with tempeh, a package we used in one of the earlier versions of our documentation. Even removing it now isn't quite possible because with sphinx-multiversion we're building all the past versions of the package into the doc, too. In a way, that nice feature comes back to haunt us there because we're basically locked into every dependency we take forever (or rather: as long as we support that version in our docs). Alternatively, one could go back to the past tagged versions, remove that dependency (and replace it or remove it) and then retag, but that's obviously work.

What I realized just now is that tempeh isn't in our requirements-dev.txt anymore! @riedgar-ms seems to have managed to just put it into our doc build script as an extra dependency. I'll try the same with bokeh now and push an update to this, or explain why it didn't work.

@romanlutz
Copy link
Member Author

Sadly, I was right with the current version of the PR. I cannot remove bokeh completely because the bokeh-plot directive is part of sphinx's extensions that are needed to build previous versions, even though it's not part of the current version. That is mildly annoying... I suppose I could go back to previous releases and remove it there also, then re-tag, but that's not as easily doable.

In summary, I don't have further changes to this and would like to merge as is (or rather: someone else should merge it) unless there are concerns. Definitely want to wait to see if @adrinjalali or @riedgar-ms have more input since you've both worked with multiversion in some capacity (and certainly more than me).

@adrinjalali
Copy link
Member

Alternatively, one could go back to the past tagged versions, remove that dependency (and replace it or remove it) and then retag

we should definitely NOT do that lol

Regarding sphinx-multiversion, I think we should replace it with a mechanism that builds the different versions completely independently of one another, and that's what we do in sklearn. Each version is completely sandboxed and the dependencies are not leaked into the other ones. We can build the docs for different versions independently, and then put them under different folders on the website, and have those links linking to different versoins.

@romanlutz
Copy link
Member Author

@adrinjalali That's interesting. I think the difference is that scikit-learn doesn't use sphinx-multiversion and I just somehow assumed it did. I will admit the scikit-learn way of building documentation is scary and looks like it'll take many hours to understand properly.

@riedgar-ms do you think there's a point in filing an issue with sphinx-multiversion or is this just fundamentally a part of how it works?

Regardless of the answer to either of these questions, shall I open an issue in our repo about this point in particular (getting multi-version builds done without having to rely on old dependencies) and we discuss it further there? I think this PR has reached its intended scope and these are just discussions beyond that. Wdyt?

@riedgar-ms
Copy link
Member

The scikit-learn doc build system looking very scary was why I was happy to come across sphinx-multiversion. I did ask about these things last summer, and I was told that sphinx-multiversion is supposed to build all versions of the documentation with the latest configuration. Which makes things like this inevitable (easy to add dependencies, but take them away, and you have to stop building that doc version); it's also the reason the conf.py has special code to detect v0.4.6 being built.

Opening an issue to review the versioned documentation build does seem wise. A related issue would be only building 'new' documentation each time, rather than always building every version (particularly if we're going to build each completely independently).

@adrinjalali adrinjalali merged commit 989cfbd into main May 6, 2021
@romanlutz romanlutz deleted the rolutz/remove_bokeh branch May 6, 2021 16:28
@romanlutz romanlutz added this to Done in Visualizations May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continues Integration and build related issues dependencies Pull requests that update a dependency file
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants