Making the dataflow temp_location argument optional#615
Making the dataflow temp_location argument optional#615zoyahav wants to merge 4 commits intoapache:python-sdkfrom zoyahav:python-sdk
Conversation
|
Hi @aaltay and @silviulica, can you please take a look? |
|
@dhalperi Dan, do you know why Travis run status is not shown in this page? I found the corresponding run at https://travis-ci.org/apache/incubator-beam/builds/143447856 |
|
I do not know -- Github. Travis. Stuff. |
|
But the build hasn't started yet, so I'm not super surprised... |
| default=None, | ||
| help='GCS path for staging code packages needed by ' | ||
| 'workers.') | ||
| # Remote execution must check that this option is not None. |
There was a problem hiding this comment.
Remote execution comment can stay. Since temp_location may still be None if staging_location is None.
And the new comment could be written without referencing Dataflow. (e.g. "If temp_location is not set, it defaults to staging_location.")
| return validator | ||
|
|
||
| test_cases = [ | ||
| {'temp_location': None, 'errors': ['temp_location']}, |
There was a problem hiding this comment.
This test matrix needs to be extended with a few staging_location, temp_location mixed inputs. Because there is a dependency between them now.
There was a problem hiding this comment.
This dependency doesn't exist in pipline_options_validator though, is there another test suit where I can add such a test?
There was a problem hiding this comment.
No, we don't have another test suite. I expect these to happen:
staging_location: valid, temp_location: None -> no errors
staging_location: invalid, temp_location: None -> staging location invalid error
staging_location: None, temp_location: None -> Error for missing arguments staging_location and temp_location
The validator already checks whether temp_location is None or not. It is doing one thing when it is not None. If it is None, it can check whether staging_location is set or not. If not could warn that these two options are missing.
There was a problem hiding this comment.
Good point, I've added this validation and some more test cases, PTAL.
|
Thank you @zoyahav. This LGTM. Let's wait for @silviulica to review before merging. |
|
LGTM - Thanks! |
|
Merged. Please manually close the PR, @zoyahav |
|
Great, thanks for the reviews! |
4 participants
Making the dataflow temp_location argument optional in the python SDK to match the java SDK.