Skip to content

[BEAM-1319] Add conflict resolution to the PipelineOptions internal argparse.#1889

Closed
aaltay wants to merge 1 commit intoapache:masterfrom
aaltay:opt
Closed

[BEAM-1319] Add conflict resolution to the PipelineOptions internal argparse.#1889
aaltay wants to merge 1 commit intoapache:masterfrom
aaltay:opt

Conversation

@aaltay
Copy link
Member

@aaltay aaltay commented Feb 1, 2017

In some instances where a PipelineOptions subclass was defined in the
main session and save_main_session option is enabled, that subclass may
appear multiple times in the PipelineOptions.subclassess() list.
This is causing problems with the argparse because options are not
unique any more.

This change filters the subclasses by name, and pick the last unique
instance of each subclass.

main session and save_main_session option is enabled, that subclass may
appear multiple times in the PipelineOptions.__subclassess__() list.
This is causing problems with the argparse because options are not
unique any more.

This changes filter the subclasses by name, and pick the last unique
instance of each subclass.
@aaltay
Copy link
Member Author

aaltay commented Feb 1, 2017

R: @chamikaramj

Re-opening #1848 as a new PR.

Added a unit test and removed the change in wordcount.py. Adding WordCountOptions work when wordcount is run only by itself but fails when running with tox. All tests after the wordcount test start requiring the output flag as defined in the wordcount options. This happens with or without the current change to the pipeline_options. We need to fix that later.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 69.644% when pulling 86c173e on aaltay:opt into 2689ca4 on apache:master.

@asfbot
Copy link

asfbot commented Feb 1, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6969/
--none--

@chamikaramj
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants