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
Fix: racially insensitive language #205
Conversation
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.
Overall looks pretty good. We might want to wait a bit to merge this because of input schema changes. I would feel more comfortable if we could test this somehow as well.
buildstockbatch/schemas/v0.1.yaml
Outdated
master_instance_type: str(required=False) | ||
slave_instance_type: str(required=False) | ||
slave_instance_count: int(min=1, required=False) | ||
manager_instance_type: str(required=False) | ||
worker_instance_type: str(required=False) | ||
worker_instance_count: int(min=1, required=False) |
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.
We shouldn't be changing old schemas as the only function they serve now is to keep a record of previous input formats.
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.
Makes sense, addressed in 517049d
buildstockbatch/schemas/v0.2.yaml
Outdated
master_instance_type: str(required=False) | ||
slave_instance_type: str(required=False) | ||
slave_instance_count: int(min=1, required=False) | ||
manager_instance_type: str(required=False) | ||
worker_instance_type: str(required=False) | ||
worker_instance_count: int(min=1, required=False) |
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.
Same here. Since we're making breaking changes to the input format, we should make a v0.3 schema and upgrade to that. I actually have a v0.3 schema going on in #187. Maybe we should wait for that to merge (soon) and make these changes there.
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.
Addressed in 517049d. That sounds good, we can hold off for the next schema to get merged.
Fixes #164
Pull Request Description
Removes racially insensitive language for EMR instance types, updating the following arguments:
master_instance_type
==>manager_instance_type
slave_instance_type
==>worker_instance_type
slave_instance_count
==>worker_instance_count
Also, now fixes the broken AWS workflow.
Checklist
Not all may apply