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

[WIP][PIP-56] Migrate the python2.7 to python3.7 #9684

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Feb 23, 2021

Fixes #8622

Motivation

Currently, ci-cpp-tests uses pulsar-build image that is from ubuntu:16.04 to build C++/Python client. The image uses libboost-all-dev for CMake to find boost dependencies. However, the Boost.Python library from Ubuntu 16.04's apt source only supports Python 2.

Modifications

  • Modify the related Dockerfile to install Boost dependency from source.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

@BewareMyPower
Copy link
Contributor Author

As I mentioned in #9682 , though I've tested the new image in my local environment, currently there's no way to verify the change of Dockerfile.

I've just pushed the image to my dockerhub: https://hub.docker.com/r/bewaremypower/pulsar-build/tags?page=1&ordering=last_updated

I'm not sure if it's proper to use the image for test. And after when all tests passed, those who have the permission of apachepulsar's permissions push the latest image.

@BewareMyPower
Copy link
Contributor Author

There's another concern that is bintray will shut down after May 2021, see https://jfrog.com/blog/into-the-sunset-bintray-jcenter-gocenter-and-chartcenter/ for details.

However, the Boost official site uses bintray to download Boost source.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

looks good
I have found the same solution googling

pulsar-client-cpp/docker-build.sh Show resolved Hide resolved
@BewareMyPower BewareMyPower changed the title [CI][C++] Use Boost.Python for Python 3 [WIP][CI][C++] Use Boost.Python for Python 3 Feb 23, 2021
@BewareMyPower
Copy link
Contributor Author

The python tests may fail because the new image use pip3 instead of pip, I'll fix it soon.

@zymap zymap marked this pull request as draft February 23, 2021 12:32
@BewareMyPower
Copy link
Contributor Author

I think it still needs some code changes to make tests pass for Python3. So I'll also remove Python2 in this PR.

@BewareMyPower
Copy link
Contributor Author

After the latest commit, the python client tests (pulsar_test.py) could pass in my docker image. However, there're still a lot to do with python functions.

For example:

======================================================================
ERROR: test_python_instance (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_python_instance
Traceback (most recent call last):
  File "/usr/lib/python3.5/unittest/loader.py", line 428, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.5/unittest/loader.py", line 369, in _get_module_from_name
    __import__(name)
  File "/pulsar/pulsar-functions/instance/target/python-instance/tests/test_python_instance.py", line 23, in <module>
    from mock import Mock
  File "/root/.local/lib/python3.5/site-packages/mock/__init__.py", line 7, in <module>
    import mock.mock as _mock
  File "/root/.local/lib/python3.5/site-packages/mock/mock.py", line 765
    mock_name = f'{self._extract_mock_name()}.{name}'
                                                    ^
SyntaxError: invalid syntax

It looks like the mock module uses the f-strings, which was introduced from Python 3.6. Therefore it requires Python >= 3.6. However the Ubuntu 16.04's python3 version is 3.5. So we need to find an alternative module.

On the other hand, I found python functions code still use some python2 legacy code.

@eolivelli
Copy link
Contributor

I believe we should stick to python2.
We cannot change the Python client and the Python support for Pulsar Functions.

This new image must be 100% compatible with the previous one regarding python

I initially added python3 in order to make the build of the image pass, because of "getpip" script.

can you try to revert all of the changes related to python ?

@BewareMyPower
Copy link
Contributor Author

BewareMyPower commented Feb 23, 2021

@eolivelli Removing Python2 support is a task of PIP 56 and issue 8622. Before the change of pulsar-build image, there were some attempts to remove Python2, like #8624, #9268

If it's not proper to do it in this PR, we need to find another way to fix the cpp/python tests. Then we cannot use Boost.Python for Python3 and should let CMake find Python2 instead of Python3.

But eventually, we need to remove the Python2 related code and tests. So I just wanted to do it in this PR.

@eolivelli
Copy link
Contributor

Agreed.

I also agree that one good way is to force cmake to find python2

@BewareMyPower
Copy link
Contributor Author

OK, I may open a new PR to do it. And I'll keep this PR and the associated branch to do the task of PIP 56.

build/docker/Dockerfile Outdated Show resolved Hide resolved
@merlimat
Copy link
Contributor

@BewareMyPower @eolivelli I'm going to re-push the old image to docker hub to unblock CI while the issue is being fixed.

We should also probably switch to a "versioned" build image, so that we can validate the switch in a PR.

@eolivelli
Copy link
Contributor

@merlimat I believe that @zymap already did it a few hours ago and he unblocked the release.
this PR is using the image from @BewareMyPower

Co-authored-by: Matteo Merli <mmerli@apache.org>
@BewareMyPower BewareMyPower changed the title [WIP][CI][C++] Use Boost.Python for Python 3 [WIP][PIP-56] Migrate the python2.7 to python3.7 Feb 24, 2021
@BewareMyPower
Copy link
Contributor Author

Since I've opened #9690 to fix the CI issue, I keeps this PR for PIP-56, which will remove Python2 from pulsar.

@codelipenghui
Copy link
Contributor

The pr had no activity for 30 days, mark with Stale label.

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

@BewareMyPower:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@BewareMyPower BewareMyPower deleted the bewaremypower/add-boost-python3 branch September 16, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate the python2.7 to python3.7
4 participants