-
Notifications
You must be signed in to change notification settings - Fork 1.2k
change: add s3_analysis_config_output_path field in DataConfig constructor #2698
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
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
For tracking purpose: per offline chat with Dewen, a SageMaker Pipeline step will use the |
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
…uctor (aws#2698) Co-authored-by: Dewen Qi <[email protected]> Co-authored-by: Shreya Pandit <[email protected]> Co-authored-by: Navin Soni <[email protected]> Co-authored-by: Ahsan Khan <[email protected]>
It looks it's not possible to parameterize model_explainability_data_config = DataConfig(
s3_data_input_path=step_process.properties.ProcessingOutputConfig.Outputs[
"shap"
].S3Output.S3Uri,
s3_output_path=ParameterString(name="s3_output_path", default_value="s3://helloworld/"),
s3_analysis_config_output_path=ParameterString(name="s3_analysis_config_output_path", default_value="s3://helloworld/analasys_config"),
label='target',
dataset_type="text/csv",
)
Nor it is possible to leave model_explainability_data_config = DataConfig(
s3_data_input_path=step_process.properties.ProcessingOutputConfig.Outputs[
"shap"
].S3Output.S3Uri,
s3_output_path=ParameterString(name="s3_output_path", default_value="s3://helloworld/"),
label='target',
dataset_type="text/csv",
)
The documenation states:
|
Issue #, if available: N/A
Description of changes:
The change is made to add
s3_analysis_config_output_path
field inDataConfig
constructor so that we can have different output path in S3 for analysis_config and processing job output.This change can benefit the implementation of Sagemaker Pipeline's Clarify step as a SageMaker Pipeline step may use the DataConfig API. The step may put placeholders (like ExecutionId or StepName) to
s3_output_path
and the placeholders can not be resolved when the API is called, but the API needs a real S3 path to upload the analysis config file. This is why the new parameters3_analysis_config_output_path
is added to accept the real S3 path.Besides, this change is backward compatible since if the
s3_analysis_config_output_path
is not supplied, thes3_output_path
will be used for the analysis_config output as previous did.Testing done: Added a unit test
Merge Checklist
Put an
x
in 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_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.