Skip to content
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

[BEAM-1251] Replace NameError-driven dispatch with past #5869

Merged
merged 1 commit into from
Jul 12, 2018

Conversation

superbobry
Copy link
Contributor

The only leftover is the ToStringCoder which uses NameError to detect Python2/3. The logic there looks fishy (it encodes text data on 2.X but not on 3.X), so I'd rather address it in a follow up PR.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
--- --- --- ---

@superbobry superbobry force-pushed the name-error-to-past branch 3 times, most recently from 0bf9395 to 217e9a0 Compare July 3, 2018 22:49
@cclauss
Copy link

cclauss commented Jul 4, 2018

This is great stuff... Can you just try:

  • flake8 . --count --show-source --statistics --select=E901,E999,F821,F822,F823 and try to whack out as many of those lingering issues as you can?

We still have
4 E999 SyntaxError: invalid syntax
11 F821 undefined name 'buffer'

Leave that 'buffer' one to me... I have nice progress on that one but all other are fair game.

@superbobry
Copy link
Contributor Author

superbobry commented Jul 4, 2018

Will do. I can fix buffer if you are short on time, just let me know.

@cclauss also, can we merged futurization PRs first?

@aaltay
Copy link
Member

aaltay commented Jul 10, 2018

Thank you @superbobry.

Could you coordinate your changes with rest of the people working on python 3 changes on the mailing list. There was a recent reviewed proposal for python 3 conversion and some of the bits you are removing were added as part of it. You can find the proposal on the Beam web site: https://beam.apache.org/contribute/#python-3-support

@tvalentyn could also help with coordination.

(I noticed that you are already started coordinating on the mailing list. That is great, please share major planned changes, such as the use of past in the mailing list.)

@cclauss
Copy link

cclauss commented Jul 10, 2018

Please resolve conflict.

@superbobry superbobry force-pushed the name-error-to-past branch 2 times, most recently from 9d87f37 to 123d2d2 Compare July 10, 2018 21:25
@superbobry
Copy link
Contributor Author

@aaltay thanks for the feedback. I've added a relevant comment on the migration proposal doc.

@cclauss done.

@tvalentyn
Copy link
Contributor

Thanks, @superbobry , this LGTM. I also ran this past coders microbenchmark in #5565 and did not see any performance impact.
@RobbeSneyders, do you have any concerns?

@RobbeSneyders
Copy link
Contributor

RobbeSneyders commented Jul 12, 2018

LGTM, thank you.
We discussed this change in the io PR here. I originally proposed to use the NameError-driven approach because of problems with typechecks when importing from builtins. However, it seems that imports from past.builtins import the native types and don't cause these problems.

The only leftover is the ``ToStringCoder`` which uses ``NameError`` to
detect Python2/3. The logic there looks fishy (it encodes text data on
2.X but not on 3.X), so I'd rather address it in a follow up PR.
@superbobry
Copy link
Contributor Author

Rebased on top of master.

Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all for making this happen!

@aaltay aaltay merged commit 730d110 into apache:master Jul 12, 2018
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.

5 participants