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.

Sign up for GitHub

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

Jump to bottom

Documentation Pipeline API Fixes #537

Merged
merged 18 commits into from
Mar 31, 2020
Merged

Documentation Pipeline API Fixes #537

merged 18 commits into from
Mar 31, 2020

Conversation

jeremyliweishih
Copy link
Collaborator

@jeremyliweishih jeremyliweishih commented Mar 30, 2020 •
edited
Loading

Fixes #385, #538 #541

Changes

  • Added docs templates for transformers, estimators and pipelines
  • Use the pipeline template for pipelines which aren't PipelineBase
  • In the templates, used autoattribute:: to list out class-level fields, and then ignore those fields in the for loop which tries to render each attribute.

Future work:

  • Figure out how to put class attributes in a table, so that they have similar formatting to instance attributes and methods

@jeremyliweishih jeremyliweishih self-assigned this Mar 30, 2020
@codecov
Copy link

codecov bot commented Mar 30, 2020 •
edited
Loading

Codecov Report

Merging #537 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #537   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files         115      115           
  Lines        4205     4205           
=======================================
  Hits         4152     4152           
  Misses         53       53
Impacted Files Coverage Δ
evalml/pipelines/components/component_base.py 88.88% <ø> (ø)
...valml/pipelines/components/estimators/estimator.py 80.00% <ø> (ø)
evalml/pipelines/pipeline_base.py 98.52% <ø> (ø)
evalml/tests/pipeline_tests/test_pipelines.py 99.62% <100.00%> (ø)

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 4cfa276...5760faa. Read the comment docs.

@dsherry dsherry mentioned this pull request Mar 30, 2020
Closed
@jeremyliweishih
Copy link
Collaborator Author

jeremyliweishih commented Mar 31, 2020 •
edited
Loading

@dsherry

Good news is I've been able to get past all the errors and warnings and surfaced class variables for pipelines and components! Heres an example:
Screen Shot 2020-03-31 at 12 52 01 AM

Bad news is... somehow I don't see the changes on RTD.

I wiped the build and am rebuilding my branch on RTD to see if that fixes anything. We should sync up on this before release!

Might be a case of: readthedocs/readthedocs.org#208
Where the API references doesn't update as RTD doesn't make a clean build.

@jeremyliweishih jeremyliweishih requested a review from dsherry March 31, 2020 04:54
@jeremyliweishih
Copy link
Collaborator Author

jeremyliweishih commented Mar 31, 2020 •
edited
Loading

RTD looks good! 😄

https://evalml.featurelabs.com/en/js_385_pdocs/generated/evalml.pipelines.RFClassificationPipeline.html

But we may want to put up an issue removing generated files before builds in RTD. This can be done in conf.py. We haven't seen any issues so far but I've looked at several repos that have done so.

@jeremyliweishih jeremyliweishih changed the title [WIP] Documentation Pipeline API Fixes Documentation Pipeline API Fixes Mar 31, 2020
@dsherry
Copy link
Contributor

dsherry commented Mar 31, 2020

@jeremyliweishih amazing!!! Will review and let's get this merged :)

dsherry
dsherry reviewed Mar 31, 2020
View reviewed changes
docs/source/api_reference.rst
:nosignatures:

get_pipelines
list_model_families
Copy link
Contributor

Choose a reason for hiding this comment

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

So we have to list out whatever utils we want to show up in the doc here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! In general if new modules or methods (unless the class or module is already in the API reference) are introduced we need to add them here -something we should look out more when reviewing (something I keep forgetting!!)

dsherry
dsherry reviewed Mar 31, 2020
View reviewed changes
docs/source/api_reference.rst
LogisticRegressionPipeline
RFRegressionPipeline
CatBoostRegressionPipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! This will fix #541 , right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup!

dsherry
dsherry reviewed Mar 31, 2020
View reviewed changes
docs/source/api_reference.rst

.. currentmodule:: evalml.pipelines.components
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome. This line will fix all the ModelFamily warnings in the docs

dsherry
dsherry reviewed Mar 31, 2020
View reviewed changes
docs/source/api_reference.rst


.. currentmodule:: evalml.model_family

Model Family
===========
============
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this necessary? I ask because if so, we should figure out if there's a linter for these files...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! It doesn't work properly unless the # of = is the same length as the text haha..

dsherry
dsherry reviewed Mar 31, 2020
View reviewed changes
docs/source/_templates/pipeline_class.rst
.. autoattribute:: model_family
.. autoattribute:: hyperparameters
.. autoattribute:: custom_hyperparameters
{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great. I agree with you that it would be nice to find a way to avoid having to list these things out here in the future. But for now, it gets things to show up in the docs, so I'm a fan. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure theres some fancy python with sphinx thing we can do in conf.py we can do in the future! Or just more features of jinja/sphinx that I'm not aware of yet.

dsherry
dsherry approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@dsherry dsherry left a comment

Choose a reason for hiding this comment

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

Left a few minor comments, but nothing really blocking except the unit test fix at the bottom. Great work on this!!! 🛳 😁

@dsherry
Copy link
Contributor

dsherry commented Mar 31, 2020

@jeremyliweishih I added to the PR description, feel free to edit as needed

This was referenced Mar 31, 2020
Merged
Closed
Closed
@jeremyliweishih jeremyliweishih mentioned this pull request Mar 31, 2020
Closed
@jeremyliweishih jeremyliweishih merged commit 7e2c7d4 into master Mar 31, 2020
@angela97lin angela97lin mentioned this pull request Apr 1, 2020
Merged
@dsherry dsherry deleted the js_385_pdocs branch October 29, 2020 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsherry dsherry dsherry approved these changes

Assignees

@jeremyliweishih jeremyliweishih

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation for Component and Pipeline API
2 participants
@jeremyliweishih @dsherry