[BEAM-1368] fixed Python SDK setup in Python 3.x #1887
Conversation
…ial recommendation for portability
wikier
changed the title
|
Changes Unknown when pulling 5253bcb on wikier:BEAM-1368_setup_python3 into ** on apache:master**. |
|
Refer to this link for build results (access rights to CI server needed): |
|
R: @chamikaramj (@wikier I assume this is ready for review, please correct me if that is not the case) |
|
Looks good. Just waiting for tests to pass. |
|
retest this please Any idea why tests are not completing here ? |
|
Yes @aaltay, should be ready to review. I have no idea why the tests are failing @chamikaramj, but doesn't look relevant to this PR. |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM |
|
R: @robertwb for merging. |
| global_names = {} | ||
| execfile(os.path.normpath('./apache_beam/version.py'), | ||
| global_names) | ||
| exec(open(os.path.normpath('./apache_beam/version.py')).read(), global_names) |
There was a problem hiding this comment.
Why in the world are we using exec here rather than just importing this file?
There was a problem hiding this comment.
I'm not sure why this was originally added and seems like we had this for some time. Might have been due to we wanting to execute this every time to capture any updates to environment instead of using a compiled version. Doesn't look like this is needed for the current usage of this file though.
There was a problem hiding this comment.
The reasons was, this executes at setup time before apache_beam is installed, and import does not work at that stage.
If there is a way to import it instead of exec I would prefer that too.
There was a problem hiding this comment.
It should work as a relative import; many packages do that. E.g. https://github.com/cython/cython/blob/master/setup.py#L254
There was a problem hiding this comment.
Makes sense, thank you.
@wikier I tried from apache_beam.utils import version and that fails with cannot import name coders. Maybe we also need to change something in apache_beam/__init__.py for the import to work.
There was a problem hiding this comment.
Right, the trick with exec is for avoiding importing the apache_beam module upon its installation.
There was a problem hiding this comment.
I suppose we could put version.py at the top level. LGTM, merging this for now as it is an improvement for Python 3, even if still not ideal.
There was a problem hiding this comment.
Opened https://issues.apache.org/jira/browse/BEAM-1431 for removing exec from the version.py.
There was a problem hiding this comment.
Not sure... the purpose of apache_beam.version is to have access to the version at runtime. The trick this PR patched was already there for the purpose I commented before. It's typical chicken-and-egg problem of setuptools.
|
What's the word on this? #4078 ? |
|
@sneurlax Python 3 conversion work is ongoing. The master issue for tracking is: https://issues.apache.org/jira/browse/BEAM-1251. If you are interested, we would be happy to accept contributions. |
7 participants
By replacing the usage of
execfile()byexec(), which is the official recommendation for portability.Further details described at BEAM-1368.
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.