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

Migrate scripts to python 3 #14798

Merged
merged 58 commits into from
Feb 17, 2020
Merged

Migrate scripts to python 3 #14798

merged 58 commits into from
Feb 17, 2020

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Nov 26, 2019

PR to identify and fix issues with python 3.

If you want to help:

  • Pick up an issue from the list of issues identified
  • If you have identified an issue that is not in the list, add it there
  • Add your name next to the item in the list (if you cannot edit this description, add a comment)
  • When you have a fix, open a PR, with feature/python3 as base branch
  • Once the issue is solved and the branch merged, check it in the list

How to use Python 3 locally:

If Python 3 is not the default in your system you may need to:

  • Remove existing virtual environments you have for beats (you can selectively remove build/python-env and build/ve directories, or just run make clean)
  • Install Python 3 in your system if it was not already installed

Issues:

Log of changes that we may consider to revert before merging:

  • Forced use of python3 using PYTHON_EXE env var
  • Don't check vendor and build directories on make check from the root directory
  • Move check to the test stage in Travis so failures there don't block other jobs so we can see their problems.

Log of issues identified/done:

To do when build is green, before merging:

Fixes #5950.

@jsoriano jsoriano self-assigned this Nov 26, 2019
@jsoriano jsoriano added the help wanted Indicates that a maintainer wants help on an issue or pull request label Nov 26, 2019
@jsoriano jsoriano added the in progress Pull request is currently in progress. label Nov 29, 2019
@kvch
Copy link
Contributor

kvch commented Dec 3, 2019

Opened small PR: #14900

kvch and others added 2 commits December 3, 2019 11:55
System tests running in docker are using the same virtual environment as
the host. If version of python in the virtual environment is not available
in the guest, it will use the default in the system, that is so far
Python 2.
@jsoriano
Copy link
Member Author

jsoriano commented Dec 4, 2019

jenkins, test this again to see if python 3 is already available in workers

@beniwohli
Copy link
Contributor

beniwohli commented Dec 6, 2019

Great stuff, fantastic to see some more Python 3 adoption!

Is the plan to switch over from Python 2 to Python 3, or to support both at the same time? If it's the former, there's a lot of changes in the PR that aren't really necessary (I assume those were introduced by 2to3 or futurize). Some examples:

  • all from __future__ imports

  • all from builtins imports

  • class SomeClass: -> class SomeClass(object) (in Python 3, all classes inherit from object, there aren't old-style classes anymore)

  • many of the the instances where an iterator is cast to a list, only to iterate over it, e.g.

     for _, deps in list(dependencies.items()):
    
  •  from future import standard_library
     standard_library.install_aliases()
    

    This installs aliases in Python 2 for modules that moved around in Python 3, so not needed if you target Python 3 only

While these changes don't really hurt, it makes the diff quite a bit larger than it could be, and the code itself is not quite as readable.

If it helps, I can open a PR that removes these unnecessary changes (again, assuming you don't plan to support running the tests with Python 2 going forward).

@andrewkroh
Copy link
Member

Ignore the OSX packaging job. The OSX packaging build needs to be disabled/removed until there's a more powerful worker available. It doesn't have enough memory to complete the build.

@jsoriano
Copy link
Member Author

jenkins, test this again please

@jsoriano jsoriano removed the in progress Pull request is currently in progress. label Feb 13, 2020
@jsoriano
Copy link
Member Author

Opened PR with the squashed changes to prepare the backport to 7.x #16302

@urso
Copy link

urso commented Feb 15, 2020

x-pack/metricbeat did fail on travis on go vet running out of memory.

Checking beats-ci and travis-ci, it looks like each testsuite did succeed at least once.

build:
context: ./_meta
args:
APPSEARCH_VERSION: ${APPSEARCH_VERSION:-7.5.0}
Copy link

Choose a reason for hiding this comment

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

The default version should be 8.0.0-SNAPSHOT, so to test against the app-search master branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Appsearch version is set to 7.5.0 also in master, we will review it as a follow up.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM. No changes needed - just pointing out my own typo.

// platforms. So do verify the version with python.exe --version.
//
// Setting up a python virtual environment on a network drive does not work
// well. So if this applies to your development environment set PYTHON_EXE
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// well. So if this applies to your development environment set PYTHON_EXE
// well. So if this applies to your development environment set PYTHON_ENV

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #16364, where I am also adding a changelog entry for this.

@jsoriano jsoriano merged commit 9375fb9 into master Feb 17, 2020
jsoriano added a commit that referenced this pull request Feb 17, 2020
Several changes in test and tools code to migrate to Python 3 and stop
supporting Python 2. From now on, Python 3 is required to run tests and
many make and mage targets.

Switch over to `python3 -m venv` instead of `virtualenv`. It's the
recommended way to create virtual Python environments in 3.4 and above.

Remove any direct references to python to always allow a specific python
binary or version to be used with the PYTHON_EXE environmet variable.

Use python3 in the shebang for python scripts. python is going to be
reserved for python2 in Ubuntu, and PEP394 mentions that python should be
used in the shebang line only for scripts that are source compatible with
both Python 2 and 3.

Update documentation to reflect these changes and add information about
the use of Python for beats development.

Prepare the tests for checks based on the local ip, this IP can be
[::1] if ipv4 is not available or 127.0.1.1 in some machines, as in the
newer Travis images.

Change checks based on sys.platform, as the values for linux have changed
in Python 3.

Several assertions in tests have been adapted to its newer versions.

Some flaky tests that are more flaky with Python 3 have been fixed or skipped.

Replace use of nosetests with mage wrapper in script for Windows CI workers.
Mage manages their own python virtual environments, this is preferred to directly
call python commands.

(cherry picked from 9375fb9)

Co-authored-by: Noémi Ványi <[email protected]>
Co-authored-by: Benjamin Wohlwend <[email protected]>
Co-authored-by: Michael Madden <[email protected]>
Co-authored-by: Andrew Kroh <[email protected]>
jsoriano added a commit that referenced this pull request Feb 18, 2020
Since the migration to Python 3 (#14798) these scripts fail in environments
not configured with unicode locales with this error. We saw this in the new
jenkins pipelines, and can be reproduced at least on Linux running make check
with LANG=C.
jsoriano added a commit to jsoriano/beats that referenced this pull request Feb 18, 2020
Since the migration to Python 3 (elastic#14798) these scripts fail in environments
not configured with unicode locales with this error. We saw this in the new
jenkins pipelines, and can be reproduced at least on Linux running make check
with LANG=C.

(cherry picked from commit c2f6358)
jsoriano added a commit that referenced this pull request Feb 18, 2020
Since the migration to Python 3 (#14798) these scripts fail in environments
not configured with unicode locales with this error. We saw this in the new
jenkins pipelines, and can be reproduced at least on Linux running make check
with LANG=C.

(cherry picked from commit c2f6358)
@simitt simitt mentioned this pull request Feb 18, 2020
4 tasks
kvch added a commit to kvch/beats that referenced this pull request Feb 20, 2020
Several changes in test and tools code to migrate to Python 3 and stop
supporting Python 2. From now on, Python 3 is required to run tests and
many make and mage targets.

Switch over to `python3 -m venv` instead of `virtualenv`. It's the
recommended way to create virtual Python environments in 3.4 and above.

Remove any direct references to python to always allow a specific python
binary or version to be used with the PYTHON_EXE environmet variable.

Use python3 in the shebang for python scripts. python is going to be
reserved for python2 in Ubuntu, and PEP394 mentions that python should be
used in the shebang line only for scripts that are source compatible with
both Python 2 and 3.

Update documentation to reflect these changes and add information about
the use of Python for beats development.

Prepare the tests for checks based on the local ip, this IP can be
[::1] if ipv4 is not available, or 127.0.1.1 in some machines, as in the
latest Ubuntu images used in Travis.

Change checks based on sys.platform, as the values for linux have changed
in Python 3.

Several assertions in tests have been adapted to its newer versions.

Some flaky tests that are more flaky with Python 3 have been fixed or skipped.

Replace use of nosetests with mage wrapper in script for Windows CI workers.
Mage manages their own python virtual environments, this is preferred to directly
call python commands.

Co-authored-by: Noémi Ványi <[email protected]>
Co-authored-by: Benjamin Wohlwend <[email protected]>
Co-authored-by: Michael Madden <[email protected]>
Co-authored-by: Andrew Kroh <[email protected]>
kvch pushed a commit to kvch/beats that referenced this pull request Feb 20, 2020
Since the migration to Python 3 (elastic#14798) these scripts fail in environments
not configured with unicode locales with this error. We saw this in the new
jenkins pipelines, and can be reproduced at least on Linux running make check
with LANG=C.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request refactoring review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor: Update our python scripts to Python 3
8 participants