docs: Add instructions to generate sphinx reference docs to CONTRIBUTING guide #178

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

Jump to bottom

Open

ca-nguyen wants to merge 4 commits into aws:main

from ca-nguyen:update-contributing-guide

Open

docs: Add instructions to generate sphinx reference docs to CONTRIBUTING guide #178

ca-nguyen wants to merge 4 commits into aws:main from ca-nguyen:update-contributing-guide

Conversation

ca-nguyen

Copy link

Contributor

@ca-nguyen ca-nguyen commented Nov 3, 2021 •

edited

Loading

Description

Updated the contributing guide to include instructions on how to generate sphinx reference documentation locally

Fixes #(issue) - N/A

Why is the change necessary?

Sphinx reference docs can't be rendered directly in your IDE and it is useful to be able to generate the docs locally to visualize your changes before merging them. For example, we use autodoc to document classes - these instructions will make it possible to visualize the documentation locally (including docstrings)

Solution

Provide instructions to generate the documentation in /doc locally based on PyCharm's Generating Reference Documentation Using Sphinx

Note: We don't need to do Tools | Sphinx quickstart since we already have a conf.py file in the /doc directory

Also in this PR:

Fixed docstring links in sagemaker.py and service.py
Added checklist item to PR template to generate sphinx documentation

Testing

Generated the documentation locally following the added instructions

Pull Request Checklist

Please check all boxes (including N/A items)

Testing

Unit tests added - N/A
Integration test added - N/A
Manual testing - why was it necessary? could it be automated? - N/A

Documentation

docs: All relevant docs updated
docstrings: All public APIs documented

Title and description

Change type: Title is prefixed with change type: and follows conventional commits
References: Indicate issues fixed via: Fixes #xxx - N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

ca-nguyen and others added 4 commits

October 28, 2021 14:18

@ca-nguyen


 Add instructions to generate sphinx reference docs to contributing guide

@ca-nguyen


 Merge branch 'main' into update-contributing-guide

237c8d8

@ca-nguyen


 Fixed sagemaker and service docstring links

7c79a6b

@ca-nguyen


 Updated PR template to include doc generation step

3d725c5

@ca-nguyen ca-nguyen mentioned this pull request

Nov 5, 2021

Add support for Amazon States Language "ResultSelector" in Task, Map ... #102

Closed

@StepFunctions-Bot

Copy link

Contributor

StepFunctions-Bot commented Nov 5, 2021

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
Commit ID: 3d725c5
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

shivlaks

shivlaks suggested changes

Nov 5, 2021

View reviewed changes

src/stepfunctions/steps/sagemaker.py

Comment on lines +228 to +229

parameters(dict, optional): The value of this field is merged with other arguments to become the request payload for SageMaker `CreateTransformJob<https://docs.aws.amazon.com/sagemaker/latest/APIReference/API_CreateTransformJob.html>`_.

You can use `parameters` to override the value provided by other arguments and specify any field's value dynamically using `Placeholders<https://aws-step-functions-data-science-sdk.readthedocs.io/en/stable/placeholders.html?highlight=placeholder#stepfunctions.inputs.Placeholder>`_.

Copy link

Contributor

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

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

curious: are these docs something that was not rendered correctly? can't tell if these changes are intended to be part of this PR or whether they address other gaps.

Copy link

Contributor Author

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

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

These changes fix the doc links - we did not catch it before, as the docs were not generated locally before merging.
docstrings need to be generated locally to validate the changes (including formatting and links)

doc/services.rst

@@ -70,11 +70,11 @@ Amazon EMR

.. autoclass:: stepfunctions.steps.service.EmrModifyInstanceGroupByNameStep

Amazon EventBridge

-----------

------------------

Copy link

Contributor

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

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

curious: is this visible to customers or is it just normalizing the style?

Copy link

Contributor Author

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

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

This addresses some warnings that came up when generating the documentation locally - i can include this in a separate PR together with the changes to fix the doc links

.github/pull_request_template.md

@@ -32,6 +32,7 @@ Please check all boxes (including N/A items)

- [ ] __docs__: All relevant [docs](https://github.com/aws/aws-step-functions-data-science-sdk-python/tree/main/doc) updated

- [ ] __docstrings__: All public APIs documented

- [ ] __Generate docs locally__: Updated docs are rendered properly (see [Contributing guide](https://github.com/aws/aws-step-functions-data-science-sdk-python/blob/main/CONTRIBUTING.md))

Copy link

Contributor

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

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

question: what does "rendered properly" mean in this context? - valid links? style? etc... can we make it more prescriptive so it isn't as subjective?

aside -> we should automate this as part of build/pipeline verification. currently, we're relying on good intentions.

CONTRIBUTING.md

#### PyCharm

1. Install Sphinx package with the Python Interpreter

1. Freeze Jinja2 package to `3.0.0rc2` - using version 3.0.0 or higher with Python 3.6 will result, when generating the docs, in errors such as :

Copy link

Contributor

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

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

3.0.0 was released after rc2.

question: why point to a release candidate. I would think that a stable release is more reliable than a pre-release version

src/stepfunctions/steps/service.py

Comment on lines +908 to +912

integration_pattern (stepfunctions.steps.integration_resources.IntegrationPattern, optional): Service integration pattern used to call the integrated service. Supported integration patterns (default: WaitForCompletion):

* WaitForCompletion: Wait for the state machine execution to complete before going to the next state. (See `Run A Job <https://docs.aws.amazon.com/step-functions/latest/dg/connect-to-resource.html#connect-sync>`_ for more details.)

* WaitForTaskToken: Wait for the state machine execution to return a task token before progressing to the next state (See `Wait for a Callback with the Task Token <https://docs.aws.amazon.com/step-functions/latest/dg/connect-to-resource.html#connect-wait-token>`_ for more details.)

* CallAndContinue: Call StartExecution and progress to the next state (See `Request Response <https://docs.aws.amazon.com/step-functions/latest/dg/connect-to-resource.html#connect-default>`_ for more details.)

Copy link

Contributor

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

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

nit: these gaps (and any others) should be addressed in their own PR as they are plugging existing gaps. they don't strictly correlate to "adding instructions"

Copy link

Contributor Author

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

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

Agreed - will move these fixes to a separate PR

@ca-nguyen ca-nguyen mentioned this pull request

Nov 22, 2021

docs: Fix doc format and links #182

Merged

7 tasks

Labels

None yet

3 participants

@ca-nguyen @StepFunctions-Bot @shivlaks

docs: Add instructions to generate sphinx reference docs to CONTRIBUTING guide #178

Are you sure you want to change the base?

docs: Add instructions to generate sphinx reference docs to CONTRIBUTING guide #178

Uh oh!

Conversation

@ca-nguyen ca-nguyen commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Why is the change necessary?

Solution

Testing

Pull Request Checklist

Testing

Documentation

Title and description

Uh oh!

StepFunctions-Bot commented Nov 5, 2021

AWS CodeBuild CI Report

Uh oh!

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

@shivlaks shivlaks Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ca-nguyen ca-nguyen Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@ca-nguyen ca-nguyen commented Nov 3, 2021 •

edited

Loading