1692 iterative graph not showing in docs #3592

MichaelFu512 · 2022-06-28T20:46:45Z

Pull Request Description

Fixes the iterative graph not showing up in docs. This is a placeholder fix because the docs do not actually load the dynamic iterative graph, so what this pr does is, after searching, it graphs all the points and displays that.

I changed all the docs that search to not show the iterative graph since it will have just shown the user empty graphs.

Closes #1692

After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

codecov · 2022-06-28T20:53:41Z

Codecov Report

Merging #3592 (7b343fe) into main (426f2b3) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3592     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        335     335             
  Lines      33381   33387      +6     
=======================================
+ Hits       33252   33258      +6     
  Misses       129     129

Impacted Files	Coverage Δ
evalml/automl/automl_search.py	`99.6% <100.0%> (+0.1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 426f2b3...7b343fe. Read the comment docs.

eccabay

Ok, I fell down into a rabbit hole of iteration plots while reviewing this, and I don't actually think this is a much of a band-aid solution as I originally thought. I left a comment about renaming the show_iteration_plot argument, and I think if we do the refactoring I mentioned there this will actually make a lot of sense.

Also, I don't think we should change the default behavior. It makes it easier to run the docs, but harder on our users to get that interactive plot. I would rather we update the docs to set show_iteration_plot (interactive_plot after renaming) to be False and add a note there mentioning this is a documentation workaround and you'll see an interactive plot by default.

eccabay · 2022-07-01T17:37:28Z

evalml/automl/automl_search.py

+                self.search_iteration_plot = self.plot.search_iteration_plot(
+                    interactive_plot=show_iteration_plot
+                )
+                return self.search_iteration_plot


Do we need to return this? Does it not render in the docs without it?

From what I remember, without returning the self.search_iteration_plot, the docs don't render.

I could use figure.show() but that causes tests to take a really long time. (https://github.com/alteryx/evalml/runs/7100924419?check_suite_focus=true)

TL;DR: the automl tests was taking over an hour when it usually takes like 20-25 minutes when I used figure.show(). When I ran the test locally, figure.show() caused every test from automl to open a web browser tab and show me the graph, multiple times (before I stopped it manually safari had like 30+ tabs of just graphs). I suspect that's the reason why it takes so long (for whatever reason).

I tried to mess around with figure.show() parameters, but couldn't figure out a way to render the graph so that:

It's interactive (and not like a jpg or png of the graph)

It doesn't make the local test open up a browser every time it runs automl.search()

Do both of the above and actually render when I build the doc using sphinx (sometimes it loads locally, then I check and it gives an error message when I upload it to github).

Of course there's a high chance I'm missing something pretty obvious (maybe I could try making it so that if figure.show() tries to use your web-browser to render it, then it doesn't?).

eccabay · 2022-07-01T17:42:18Z

evalml/automl/automl_search.py

+            if not show_iteration_plot:
+                self.search_iteration_plot = self.plot.search_iteration_plot(
+                    interactive_plot=show_iteration_plot
+                )


I had a really hard time understanding what this does. After falling down an iteration_plot rabbit hole, I think we should rename the show_iteration_plot argument to interactive_plot, to match the arg that's passed into search_iteration_plot.

In my mind, that's how this logic makes sense. If interactive_plot is True, we call self.plot.search_iteration_plot up around line 910, which will create the interactive plot as we know and love. If interactive_plot is False and verbose is True (the other scenario where we'd want a plot), we run this code at the end to generate that final post-search plot that the docs require.

Could you refactor this section and the section around line 910 to follow that logic? It'd keep things much simpler and easier to follow.

To be a little clearer here, I think this section could look like

if self.verbose and not interactive_plot: self.search_iteration_plot = self.plot.search_iteration_plot(interactive_plot=interactive_plot)

eccabay · 2022-07-01T17:42:35Z

evalml/automl/automl_search.py

+            # go = import_or_raise(
+            #     "plotly.graph_objects",
+            #     error_msg="Cannot find dependency plotly.graph_objects",
+            # )
+            # title = "Pipeline Search: Iteration vs. {}<br><sub>Gray marker indicates the score at current iteration</sub>".format(
+            #     self.objective.name
+            # )
+            # layout = {
+            #     "title": title,
+            #     "xaxis": {"title": "Iteration", "rangemode": "tozero"},
+            #     "yaxis": {"title": "Score"},
+            # }


We don't need commented out code!

chukarsten

I think Becca's left some good comments for addressing. Please keep an eye out for commiting a commented code. We find that a good way to prevent this is, while your PR is in draft status, doing a PR review on yourself and look over the diff. Review your PR as if you were an approver before changing status to ready!

…/alteryx/evalml into 1692-Iterative-Graph-Not-Showing

MichaelFu512 · 2022-07-05T22:18:33Z

docs/source/user_guide/automl.ipynb

+  },
+  "vscode": {
+   "interpreter": {
+    "hash": "425958b484b95e5a5ab632cc76b33302e47efa72ef6819296d6f51644ebb328b"


I don't recall adding any of this, I wonder where it came from?

did you open the notebook using vscode? Not sure if make lint-fix covers this part of the notebook but you could always manually remove it by opening the notebook in a text editor.

I did open the notebook using vscode. I'll remove these from now on.

MichaelFu512 · 2022-07-05T22:19:16Z

evalml/automl/automl_search.py

+                self.search_iteration_plot = self.plot.search_iteration_plot(
+                    interactive_plot=interactive_plot
+                )
+                if pio.renderers.default != "browser":


This stops the automl tests from mass opening graphs in the browser, which was causing the tests to be really slow.

MichaelFu512 · 2022-07-05T22:19:58Z

docs/source/demos/fraud.ipynb

@@ -281,7 +281,7 @@
 ],
 "metadata": {
  "kernelspec": {
-   "display_name": "Python 3",
+   "display_name": "Python 3 (ipykernel)",


Not sure where these metadata changes are coming from.

I have the same issue - if anyone has a way to fix this let me know, my current solution is to edit the file here in GitHub to remove these (same with the vscode hash down below).

eccabay

Looks good, thanks for making these changes!

eccabay · 2022-07-06T14:21:46Z

docs/source/demos/fraud.ipynb

@@ -281,7 +281,7 @@
 ],
 "metadata": {
  "kernelspec": {
-   "display_name": "Python 3",
+   "display_name": "Python 3 (ipykernel)",


I have the same issue - if anyone has a way to fix this let me know, my current solution is to edit the file here in GitHub to remove these (same with the vscode hash down below).

eccabay · 2022-07-06T14:23:01Z

docs/source/user_guide/automl.ipynb

@@ -979,7 +979,7 @@
 ],
 "metadata": {
  "kernelspec": {
-   "display_name": "Python 3",
+   "display_name": "Python 3.8.13 ('test123')",


How did this get here?

eccabay · 2022-07-07T15:16:02Z

evalml/automl/automl_search.py

+                )
+                if pio.renderers.default != "browser":
+                    self.search_iteration_plot.show()
+                # return self.search_iteration_plot


This comment should be removed!

Aghhhh, I somehow missed that.

eccabay · 2022-07-07T15:16:35Z

evalml/automl/automl_search.py

+
+        Returns:
+            A plotly figure of the completed search


With using the browser check instead, we no longer need to have this in the docstring!

chukarsten

Looks good, thanks Michael!

MichaelFu512 added 7 commits June 28, 2022 09:02

Shows two graphs, one empty one filled

476b3ec

makefile changes to remove -j auto

7f38eae

Made it only print out one graph

45e520d

Fixed linting

dea7d41

Updated docstring

3d2066b

Put back the thing I edited inside makefile

a92adfe

Fixed linting + makefile error

040555e

MichaelFu512 added 13 commits June 28, 2022 14:51

test to see if .show() makes tests slow

706fb93

oops fixed it

5c99965

Trying renderer="png" to see if tests faster

b629234

release notes update

18dc026

testing to see if returning the figure works

0dfb423

At this point something has to work.

beee939

make html has the graph correct

2deea88

Fixed linting and reverted my docstring changes

dbd6371

Trying to make the parameter false.

9a78ce3

Linting issues

8c14958

show_iteration_plot is now default false

2c105d5

Added testing and removed redundant code

c03c029

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

4b468cf

MichaelFu512 marked this pull request as ready for review June 29, 2022 20:25

auto-assign bot assigned MichaelFu512 Jun 29, 2022

MichaelFu512 requested review from chukarsten, jeremyliweishih, eccabay and christopherbunn June 29, 2022 20:26

MichaelFu512 added 2 commits June 30, 2022 09:01

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

d3c9fbb

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

f236566

eccabay requested changes Jul 1, 2022

View reviewed changes

chukarsten reviewed Jul 5, 2022

View reviewed changes

MichaelFu512 added 9 commits July 5, 2022 08:41

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

7edf9a4

Added Becca's changes

f15f078

Added check so that tests don't take hours

d6ad7b6

Edited some ipynb to reflect change

f188e01

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

8c67383

Linting issue

f2fe412

Merge branch '1692-Iterative-Graph-Not-Showing' of https://github.com…

862f4b1

…/alteryx/evalml into 1692-Iterative-Graph-Not-Showing

Documentation fixes

277c338

Deleted un-needed test

1159222

MichaelFu512 commented Jul 5, 2022

View reviewed changes

Merge branch 'main' into 1692-Iterative-Graph-Not-Showing

0cd9027

MichaelFu512 requested a review from eccabay July 5, 2022 22:57

MichaelFu512 enabled auto-merge (squash) July 5, 2022 23:26

eccabay approved these changes Jul 7, 2022

View reviewed changes

MichaelFu512 added 4 commits July 7, 2022 09:05

Removed metadata from fraud.ipynb

5439a54

Removed metadata from start.ipynb

b2a1ffc

Removed metadata from automl.ipynb

666e274

Updated automl_search.py

7b343fe

chukarsten approved these changes Jul 7, 2022

View reviewed changes

MichaelFu512 merged commit 26896e9 into main Jul 7, 2022

MichaelFu512 deleted the 1692-Iterative-Graph-Not-Showing branch July 7, 2022 20:19

MichaelFu512 restored the 1692-Iterative-Graph-Not-Showing branch July 7, 2022 21:22

chukarsten deleted the 1692-Iterative-Graph-Not-Showing branch July 7, 2022 23:37

chukarsten mentioned this pull request Jul 24, 2022

Release v0.55.0 #3625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1692 iterative graph not showing in docs #3592

1692 iterative graph not showing in docs #3592

MichaelFu512 commented Jun 28, 2022 •

edited

codecov bot commented Jun 28, 2022 •

edited

eccabay left a comment

eccabay Jul 1, 2022

MichaelFu512 Jul 5, 2022 •

edited

eccabay Jul 1, 2022

eccabay Jul 1, 2022 •

edited

eccabay Jul 1, 2022

chukarsten left a comment

MichaelFu512 Jul 5, 2022

jeremyliweishih Jul 6, 2022

MichaelFu512 Jul 7, 2022

MichaelFu512 Jul 5, 2022

MichaelFu512 Jul 5, 2022

eccabay Jul 6, 2022

eccabay left a comment

eccabay Jul 6, 2022

eccabay Jul 6, 2022

eccabay Jul 7, 2022

MichaelFu512 Jul 7, 2022

eccabay Jul 7, 2022

chukarsten left a comment

1692 iterative graph not showing in docs #3592

1692 iterative graph not showing in docs #3592

Conversation

MichaelFu512 commented Jun 28, 2022 • edited

Pull Request Description

codecov bot commented Jun 28, 2022 • edited

Codecov Report

eccabay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelFu512 Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eccabay Jul 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chukarsten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eccabay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chukarsten left a comment

Choose a reason for hiding this comment

MichaelFu512 commented Jun 28, 2022 •

edited

codecov bot commented Jun 28, 2022 •

edited

MichaelFu512 Jul 5, 2022 •

edited

eccabay Jul 1, 2022 •

edited