feature: Add support for Partial Dependence Plots(PDP) in SageMaker Clarify #2716
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
1 similar comment
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
tox.ini
Outdated
| @@ -140,7 +140,7 @@ setenv = | |||
| LANG=C.UTF-8 | |||
| deps = black | |||
| commands = | |||
| black -l 100 --check ./ | |||
| black -l 100 ./ | |||
There was a problem hiding this comment.
is it by mistake ? The removal of --check ?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
keerthanvasist
dismissed stale reviews from jeniyat and karan6181
via
261a3a6
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
src/sagemaker/clarify.py
Outdated
| analysis_config["methods"] = explainability_config.get_explainability_config() | ||
|
|
||
| explainability_methods = {} | ||
| if isinstance(explainability_config, List): # pylint: disable=W1116 |
There was a problem hiding this comment.
this should be "list", List is from typing.
src/sagemaker/clarify.py
Outdated
| @@ -772,7 +828,8 @@ def run_explainability( | |||
| model_config (:class:`~sagemaker.clarify.ModelConfig`): Config of the model and its | |||
| endpoint to be created. | |||
| explainability_config (:class:`~sagemaker.clarify.ExplainabilityConfig`): Config of the | |||
| specific explainability method. Currently, only SHAP is supported. | |||
| specific explainability method or a list of ExplainabilityConfig objects. Currently, | |||
There was a problem hiding this comment.
why don't you document that it can be a list also?
tests/integ/test_clarify.py
Outdated
| @@ -258,6 +262,57 @@ def test_shap(clarify_processor, data_config, model_config, shap_config, sagemak | |||
| check_analysis_config(data_config, sagemaker_session, "shap") | |||
|
|
|||
|
|
|||
| def test_pdp(clarify_processor, data_config, model_config, pdp_config, sagemaker_session): | |||
| with timeout.timeout(minutes=CLARIFY_DEFAULT_TIMEOUT_MINUTES): | |||
There was a problem hiding this comment.
is this running a real job?
There was a problem hiding this comment.
Yes. I removed the new integration tests added.
tox.ini
Outdated
| @@ -46,7 +46,7 @@ ignore = | |||
| require-code = True | |||
|
|
|||
| [doc8] | |||
| ignore-path=.tox,src/sagemaker.egg-info | |||
| ignore-path=.tox,src/sagemaker.egg-info,venv | |||
There was a problem hiding this comment.
I feel this change should be removed. I guess you use this venv folder for Python virtual env, it is your own setup, not part of the standard setup recommended by contribution guide. (As a workaround you can use a different path, like ~/.venv)
There was a problem hiding this comment.
+1, please remove this change.
There was a problem hiding this comment.
I have removed this change. I chose to leave it there because a venv is excluded for other commands in the same file, and in gitignore as well. It would have been a harmless convenience
There was a problem hiding this comment.
venv is pretty common, maybe make it a separate commit?
src/sagemaker/clarify.py
Outdated
| @@ -54,7 +56,11 @@ def __init__( | |||
| "ShardedByS3Key". | |||
| s3_compression_type (str): Valid options are "None" or "Gzip". | |||
| """ | |||
| if dataset_type not in ["text/csv", "application/jsonlines", "application/x-parquet"]: | |||
| if dataset_type not in [ | |||
There was a problem hiding this comment.
General comment: my suggestion is that you undo code formatting like because they are not necessary and distract the reviewers, ideally you should only make necessary changes for the feature. (If the code is formatted automatically by your IDE then reset the IDE settings)
There was a problem hiding this comment.
I removed this for now. There are multiple formatting checks (pylint, black etc) which check on push too. It is hard for developer also to figure out which change was cause by IDE and caused by formatters.
There was a problem hiding this comment.
Then try harder not to let your IDE cause trouble.
src/sagemaker/clarify.py
Outdated
|
|
||
| def get_explainability_config(self): | ||
| """Returns config.""" | ||
| return {"pdp": copy.deepcopy(self.pdp_config)} |
There was a problem hiding this comment.
Make a copy of the entire return value. (so that any modification to the return value by the caller won't impact the PDPConfig object.)
copy.deepcopy({"pdp": self.pdp_config})
There was a problem hiding this comment.
I have updated this, although I see no difference between the two.
There was a problem hiding this comment.
There is. In below case do you expect config_dict2 to have the update made for config_dict?
config_dict = pdp_config.get_explainability_config()
config_dict["pdp"]["asdf"] = 1
config_dict2 = pdp_config.get_explainability_config()
There was a problem hiding this comment.
You are right. My concern was the caller may update the return value and impact the original class, but in this case there seems no impact. It’s fine as lone as there is no impact.
src/sagemaker/clarify.py
Outdated
| if not isinstance(config, ExplainabilityConfig): | ||
| raise ValueError( | ||
| f"Invalid input: Excepted ExplainabilityConfig, got {type(config)} instead" | ||
| ) |
There was a problem hiding this comment.
Although the isinstance(config, ExplainabilityConfig) check allows raising a better error/message to the customer, it is not necessary because if explainability_config is invalid then explainability_config.get_explainability_config() should fail anyway.
src/sagemaker/clarify.py
Outdated
| explainability_methods[list(explain_config.keys())[0]] = explain_config[ | ||
| list(explain_config.keys())[0] | ||
| ] | ||
| elif isinstance(explainability_config, ExplainabilityConfig): |
There was a problem hiding this comment.
I think it should be simply a "else:", then the code fail fast here if an invalid explainability_config parameter is passed to the method. Better than sending an empty "methods" list to server side for validation.
src/sagemaker/clarify.py
Outdated
| explainability_methods[list(explain_config.keys())[0]] = explain_config[ | ||
| list(explain_config.keys())[0] | ||
| ] |
There was a problem hiding this comment.
explainability_methods.update(explain_config) should do the job.
And there should be validations after the for loop
- validate the length of the result it shall not be zero
- validate the length of the result to make sure that there was no overwrite,
assert len(explainability_methods.keys())>0, "Please provide at least one explaianbility config."
assert len(explainability_methods.keys())==len(explainability_config) "There are duplicate explainability configs"
| @@ -574,6 +598,34 @@ def _run_test_shap( | |||
| ) | |||
|
|
|||
|
|
|||
| @patch("sagemaker.utils.name_from_base", return_value=JOB_NAME) | |||
| def test_pdp( | |||
There was a problem hiding this comment.
Ideally there should also be test cases for invalid input, for example:
- explainability_config is None
- explainability_config is an emtpy list
- Multiple shap_config (or pdp_config) in explainability_config
There was a problem hiding this comment.
Done. Additional tests are added.
tests/integ/test_clarify.py
Outdated
| def test_shap_and_pdp( | ||
| clarify_processor, data_config, model_config, shap_config, pdp_config, sagemaker_session | ||
| ): | ||
| with timeout.timeout(minutes=CLARIFY_DEFAULT_TIMEOUT_MINUTES): |
There was a problem hiding this comment.
As I remember pdp is time consuming as well as shap, if we run both in the same job then would the job runtime be close to the CLARIFY_DEFAULT_TIMEOUT_MINUTES? Considering that the time to launch processing instances and create shadow endpoint is not a fixed value, we should leave enough buffer for them to avoid flaky test. My suggestion is that you run this test case using your indivisual account to see the runtime and make a decision (on keeping the timeout value or increase it).
There was a problem hiding this comment.
Thanks for contributing to SM PySDK.
15 Minutes timeout is a very long time for an integ test run. What is the actual runtime expected here?
We should be looking into ways to reduce this runtime and be frugal here. These integ tests currently run for every PR to PySDK and would add that much of a buffer causing our PR's to be queued up.
There was a problem hiding this comment.
I did run these tests, and they pass within the timeout, but I also don't think they are super useful. So, I did away with all the new integ tests I added in this CR. Unit tests cover the changes I have made well. The only change is the what analysis_config is to be generated. Beyond that, clarify container integration tests will cover it.
If this is okay, I am also okay with this. However, we cannot reduce the runtime of these integration tests because bringing up a processing job takes time.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
src/sagemaker/clarify.py
Outdated
| assert ( | ||
| len(explainability_config) > 0 | ||
| ), "Please provide at least one explaianbility config." | ||
| for config in explainability_config: | ||
| explain_config = config.get_explainability_config() | ||
| explainability_methods.update(explain_config) | ||
| assert len(explainability_methods.keys()) == len( | ||
| explainability_config | ||
| ), "There are duplicate explainability configs" | ||
| else: |
There was a problem hiding this comment.
I would prefer to raise ValueErrors or some other meaningful errors here instead of AssertionError. AssertionError belongs to cases where you test inner behaviour and not make user-sanity checks.
There was a problem hiding this comment.
Will do. Parking this PR until a fix goes in. Will update again next week.
There was a problem hiding this comment.
Updated. This CR now ready.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…larify (aws#2716) Co-authored-by: Navin Soni <[email protected]>
Description of changes:
Add support for Partial Dependence Plots(PDP) in SageMaker Clarify
Testing done:
Added unit and integration tests.
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_baseto 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.