Skip to content

[BEAM-1368] fixed Python SDK setup in Python 3.x #1887

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.

Sign up for GitHub

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

Closed
wikier wants to merge 1 commit into from
Closed

[BEAM-1368] fixed Python SDK setup in Python 3.x #1887

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions sdks/python/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@

def get_version():
global_names = {}
execfile(os.path.normpath('./apache_beam/version.py'),
global_names)
exec(open(os.path.normpath('./apache_beam/version.py')).read(), global_names)
Copy link
Contributor

@robertwb robertwb Feb 3, 2017

Choose a reason for hiding this comment

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

Why in the world are we using exec here rather than just importing this file?

Copy link
Contributor

@chamikaramj chamikaramj Feb 3, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aaltay aaltay Feb 3, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@robertwb robertwb Feb 4, 2017

Choose a reason for hiding this comment

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

It should work as a relative import; many packages do that. E.g. https://github.com/cython/cython/blob/master/setup.py#L254

Copy link
Member

@aaltay aaltay Feb 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@wikier wikier Feb 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right, the trick with exec is for avoiding importing the apache_beam module upon its installation.

Copy link
Contributor

@robertwb robertwb Feb 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aaltay aaltay Feb 8, 2017

Choose a reason for hiding this comment

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

Opened https://issues.apache.org/jira/browse/BEAM-1431 for removing exec from the version.py.

Copy link
Member Author

@wikier wikier Feb 8, 2017

Choose a reason for hiding this comment

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

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.

return global_names['__version__']

PACKAGE_NAME = 'apache-beam-sdk'
Expand Down