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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ca-nguyen
Copy link
Contributor

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

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.

@StepFunctions-Bot
Copy link
Contributor

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

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

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

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)

@@ -70,11 +70,11 @@ Amazon EMR
.. autoclass:: stepfunctions.steps.service.EmrModifyInstanceGroupByNameStep

Amazon EventBridge
-----------
------------------
Copy link
Contributor

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

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

@@ -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

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.


#### 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

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

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

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

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
7 tasks
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.

3 participants