-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feature: allow setting the default bucket in Session #1168
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
feature: allow setting the default bucket in Session #1168
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 |
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 |
|
||
input_file_path = os.path.join(DATA_DIR, "dummy_input.txt") | ||
|
||
sklearn_processor = SKLearnProcessor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: since this functionality lives in the parent Processor
and propagates down to child classes, the test should use the Processor
class rather than SKLearnProcessor
. (If we validate using Processor
, we validate for all child classes, but if we validate using SKLearnProcessor
, we don't validate for parent classes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it the other way around.
I prefer to use the implementation in this case (SKLearnProcessor), because it allows us to validate the superclass inherited methods (normalization of inputs) AS WELL as the custom implementation logic (automatic code upload).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ ah, I realize I should've said ScriptProcessor
, not Processor
-- if another child class of ScriptProcessor
is added, and if the integ tests use ScriptProcessor
, automatic code upload would be tested for that new class by dint of it being tested with ScriptProcessor
. (For example: what if SKLearnProcessor
has some code added to it that lets this test pass just for SKLearnProcessor?)
This reverts commit 98fa76d.
Issue: #1165
Description of changes: Session now allows default bucket name to be passed in. Particularly useful in controlled environment where new buckets cannot be created.
Testing done: New tests added to validate expected custom bucket
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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.