Skip to content

documentation: add detail & links to clarify docstrings #3216

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
Merged
merged 1 commit into from
Jul 8, 2022
Merged

documentation: add detail & links to clarify docstrings #3216

merged 1 commit into from
Jul 8, 2022

Conversation

samsaranc
Copy link
Contributor

Issue #
https://sim.amazon.com/issues/RAI-2029

Summary of Changes

Minor changes to the docstrings of clarify.py to adhere to the SageMaker Python SDK Documentation Guidelines and keep information up-to-date.

Specifically:

  • make language more precise and grammatically comply with Doc Guidelines (present tense, fix typos)
  • add missing API references to other Clarify & SageMaker classes
  • specify default behavior (& numerical parameters) for ImageConfig parameters
  • briefly define complex terms (SHAP, PDP, etc) & add links to their pages in the Clarify developer guide
  • add "Raises" annotations to methods that throw exceptions
  • add links to external open-source algorithms used (ex: SKLearn)
  • annotate parameter names, enums, and raw text for readability
  • add description of PDP to the run_explainability function docstring (only SHAP was listed, even though PDP is already supported in the function definition)

Testing done:

  • ran unit tests
  • ran tox -e twine, sphinx and manually reviewed my changes to sagemaker-python-sdk/doc/_build/html/api/training/processing.html before committing & making this PR

Merge Checklist

General

  • I have read the CONTRIBUTING doc
  • I certify that the changes I am introducing will be backwards compatible, and I have discussed concerns about this, if any, with the Python SDK team
  • I used the commit message format described in CONTRIBUTING
  • I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
  • I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

** NOTE: I skipped adding tests because this was just a docs update**

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
  • I have checked that my tests are not configured for a specific region or account (if appropriate)
  • I have used unique_name_from_base to create resource names in integ tests (if appropriate)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 2c81c57
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 2c81c57
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 2c81c57
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 2c81c57
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: 2c81c57
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 34f6669
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 34f6669
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 34f6669
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: 34f6669
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 34f6669
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: d03e012
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: d03e012
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: d03e012
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-slow-tests
  • Commit ID: d03e012
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@codecov-commenter
Copy link

Codecov Report

Merging #3216 (d03e012) into master (46c68a9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3216   +/-   ##
=======================================
  Coverage   88.79%   88.79%           
=======================================
  Files         202      202           
  Lines       17833    17833           
=======================================
  Hits        15835    15835           
  Misses       1998     1998
Impacted Files Coverage Δ
src/sagemaker/clarify.py 95.02% <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 46c68a9...d03e012. Read the comment docs.

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: d03e012
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mufaddal-rohawala mufaddal-rohawala merged commit 4879ec3 into aws:master Jul 8, 2022
keerthanvasist
keerthanvasist reviewed Jul 8, 2022
View reviewed changes
src/sagemaker/clarify.py
If this field is None, then the ``s3_output_path`` will be used
to store the ``analysis_config`` output.
label (str): Target attribute of the model **required** for bias metrics (both pre-
and post-training). Optional when running SHAP explainability.
Copy link
Member

@keerthanvasist keerthanvasist Jul 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is also required for SHAP. The only time it's okay to not give label is when the dataset does not contain the label (which is okay for explainability and not bias metrics). We need the label for explanability too because we need to remove it from the dataset if present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, added this to my upcoming PR.

src/sagemaker/clarify.py
Model inference is run to see how the prediction changes with the replaced features.
If the model output returns multiple scores importance is computed for each of them.
Across examples, feature importance is aggregated using 'agg_method'.

Copy link
Member

Choose a reason for hiding this comment

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

SHAP and PDP could be requested together as well. Maybe try to reflect that as well here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, added this to my upcoming PR.

jerrypeng7773 pushed a commit to jerrypeng7773/sagemaker-python-sdk that referenced this pull request Aug 8, 2022
@samsaranc @jerrypeng7773
documentation: add detail & links to clarify docstrings (aws#3216)
00e187e
JoseJuan98 pushed a commit to JoseJuan98/sagemaker-python-sdk that referenced this pull request Mar 4, 2023
@samsaranc @JoseJuan98
documentation: add detail & links to clarify docstrings (aws#3216)
1164c60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keerthanvasist keerthanvasist keerthanvasist left review comments

@shreyapandit shreyapandit shreyapandit approved these changes

@mufaddal-rohawala mufaddal-rohawala mufaddal-rohawala approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@samsaranc @sagemaker-bot @codecov-commenter @shreyapandit @keerthanvasist @mufaddal-rohawala