[CD] PyPI pipeline fix

[perdasilva]

Description

PyPI CD pipeline is failing - it seems base.py should be made python2 compatible. The call to open with encoding is breaking compatibility. This PR makes that update. A successful run on dev can be seen here.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
• Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
• Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• For new C++ functions in header files, their functionalities and arguments are documented.
• For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
• Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
• To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

[haojin2]

@szha any comments?

[lanking520]

LGTM

[lanking520]

But one more thing. Does this have problem with Python2?

[perdasilva]

@lanking520

It seems to be the case. When trying to package with python2, we get:

[GCC 5.4.0 20160609]. Python >= 3.5 is required to make the signatures display correctly.
  .format(str(sys.version)))
Traceback (most recent call last):
  File "setup.py", line 170, in <module>
    _generate_op_module_signature('mxnet', 'symbol', _generate_symbol_function_code)
  File "/work/mxnet/wheel_build/mxnet/base.py", line 704, in _generate_op_module_signature
    module_op_file = get_module_file("%s.%s.op" % (root_namespace, module_name))
  File "/work/mxnet/wheel_build/mxnet/base.py", line 675, in get_module_file
    module_file = open(file_name, 'w', encoding="utf-8")
TypeError: 'encoding' is an invalid keyword argument for this function

[szha]

The problem here is that open(encoding=...) is not valid in python2. Given that we won't be dropping python2 support until MXNet 2.0 (see #8703), this usage should have been prevented.

The illegal usage is at https://github.com/apache/incubator-mxnet/blame/master/python/mxnet/base.py#L675, which was merged as part of the #16738 (cc @yajiedesign @TaoLv). To ensure that python2 support is not dropped because of this, a more portable method should be used (e.g. io.open).

The next question I have is, how come the CI didn't catch such problem? Do we no longer have python2 validation in the CI? (cc @marcoabreu)

[szha]

@aaronmarkham @lanking520 @larroy make sure your approvals are better informed next time.

[marcoabreu]

Python2 in general is still active, but since we don't have test coverage support, I'm unable to determine whether that specific line is hit. Since python will only throw an error if the actual line is interpreted, it's quite likely that we didn't hit it actually.

In general, I'd recommend some type of static analysis (I think flake8 is capable of doing it) do verify that our code base is python2 compatible. So far, we don't have that yet though.

[szha]

@marcoabreu agreed. The simple static analysis can be added as part of the sanity check stage in the pipelines.

[TaoLv]

python3 is called explicitly at pylint here:
https://github.com/apache/incubator-mxnet/blob/master/Makefile#L663.

[marcoabreu]

It was switched to python3 here: #14807

[aaronmarkham]

@aaronmarkham @lanking520 @larroy make sure your approvals are better informed next time.

My bad. I agree that I'm not fully informed on every aspect of creating packages, and took it for granted the the OP was informed due to the apparent successful tests. I'll give him much latitude and trust, and even risk not having to nanny the PR by running the tests myself. But, I think you're conflating issues. The publish process was tested by the OP, and the change aligns with my understanding of our common longer term goals. Changes that impacted Python 2 support in an unrelated PR were not in scope in the review this particular PR. Nonetheless, I appreciate that you took the time to also review this PR as you're most knowledgable about creating the pip packages.

Let's clarify what's going on here please.

If the pipeline is to make artifacts, why can't it do that using Python3 only? Can it not suffice for any artifact building of the packages, or must we build the Python 2 packages with all Python 2 tooling?

We have other situations where we build artifacts in one realm, and I have been using every opportunity to add Python3 capability, and for new stuff, default to Python3. If this is not the case, it should be brought up on dev@ as it's a bit of a philosophical switch. Plus, if I plan to contribute a tutorial or example or CI tooling, as long as it doesn't break something else, I'm in no obligation to make it Python 2 compatible, am I?

[marcoabreu]

For examples and tutorials I'd generally say to also consider python2 support for the sake of user experience. For internal tooling though it's totally up to us.

[szha]

@aaronmarkham the committers are responsible for the quality of the software, and code review is the standard way to ensure the quality. Approvals should only be provided for code that you are familiar with and can be responsible for. Such responsibility cannot be shifted to the contributors regardless of whether you have good intention or trust to the contributor.

With respect to this PR, a past problem was surfaced in the CD pipeline, and the change will bury the problem again, so it definitely doesn’t fall in the category of “not in scope”. When an issue surfaces, either fixing the original issue or making other changes to bury it would both make the issue go away, but it should be obvious that the first approach should be taken, and the committers should recommend so.

I’ve linked the past RFC on deprecation plan for python 2. So far the agreement is on keeping the mxnet support until 2.0. The rest peripherals are not defined and you can make your best judgement.

[perdasilva]

Made it python2 compatible. Successful example on dev

[aaronmarkham]

Changing to just a comment instead of approval. Since we do know that we don't know if other code from some previous PR that may or may not impact Python2 functionality, I know that I am not confident that your successful tests is enough evidence that this PR can be accepted. I wish you luck on whatever other hurdles you may have on this PR.

not informed

[marcoabreu]

Per, would it be okay with you to quickly run pylint2 manually on your computer? If everything is okay, I'm happy to merge your PR.

I'm not sure if pylint also checks if calling signatures are verified, so if you know a tool that verifies it, that'd be great. There's specifically no need for this PR to add an automation or anything, it's just about sanity checking for now to make sure that the package will actually not be broken.

[perdasilva]

Hey @marcoabreu

I didn't quite get it. I've added some changes to disable this rule for that specific line.
If I understood you correctly, you wanted me to run pylint with python2?

Seems to work with python2 (without my changes):

python2 -m pylint --rcfile=ci/other/pylintrc --ignore-patterns=".*\.so$,.*\.dll$,.*\.dylib$" python/mxnet/base.py
Using config file /Users/silvaper/repositories/perdasilva/incubator-mxnet/ci/other/pylintrc

------------------------------------
Your code has been rated at 10.00/10

(python3 just to sanity check that the changes weren't there =D)

python3 -m pylint --rcfile=ci/other/pylintrc --ignore-patterns=".*\.so$,.*\.dll$,.*\.dylib$" python/mxnet/base.py
************* Module mxnet.base
python/mxnet/base.py:92:35: E0602: Undefined variable 'unicode' (undefined-variable)

-------------------------------------------------------------------
Your code has been rated at 9.88/10 (previous run: 10.00/10, -0.12)

With my changes, it seems to work with both:

python2 -m pylint --rcfile=ci/other/pylintrc --ignore-patterns=".*\.so$,.*\.dll$,.*\.dylib$" python/mxnet/base.py
Using config file /Users/silvaper/repositories/perdasilva/incubator-mxnet/ci/other/pylintrc

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

python3 -m pylint --rcfile=ci/other/pylintrc --ignore-patterns=".*\.so$,.*\.dll$,.*\.dylib$" python/mxnet/base.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Let's see if the sanity check agrees ^^

[perdasilva]

Regarding the call signature check, I'm really not sure. I've only managed to find this, which suggests that maybe it does?

[perdasilva]

Hmm...but maybe this is just number of arguments...not sure if it checks type annotations etc ..