1428705 - [manifestparser] Add support for Python 3

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Without dropping support for legacy Python, we need to add support for Python 3 to manifestparser.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

To work on this bug you will need to install and configure Mercurial, which will enable you to download the Firefox source code. It will also be used to commit your changes locally and prepare a patch for review. See the installation guide at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/installing.html for help getting Mercurial on your system. Once installed, there are some extensions we recommend installing, details of these can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/extensions.html

To clone the Firefox source code we recommend using the unified repository. Details of this and how to create a bookmark for your work can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html

As this bug relates to Python 3, you will need to have this installed on your system. Our continuous integration is currently using Python 3.5, so for best results we recommend using the same version locally. There are a number of ways to install Python, and they vary depending on your environment. We suggest reading over the guides at http://docs.python-guide.org/en/latest/starting/installation/#python-3-installation-guides to find the best method for you. Note that we also need to maintain support for Python 2, so you'll also need to have Python 2.7 installed.

Whilst we're moving towards adding support for Python 3, we've disabled any tests that fail against this version. This means that in order to work on this bug you will need to enable the tests. This can be done by removing "skip-if = python == 3" from the manifest file in `testing/mozbase/manifestparser/tests/manifest.ini`

To run the tests against Python 3, execute the following command:

```
mach python-test --python=3.5 testing/mozbase/manifestparser
```

Work through the test failures, and update the tests and source code for the package to simultaneuously support Python 2.7 and Python 3.5. You can always check that your changes have not inadvertantly broken support for Python 2 by using `--python=2.7` on the command line. 

If you're new to Python 3, the guide at https://docs.python.org/3/howto/pyporting.html may help you to get started with understanding the differences. To assist with supporting both Python 2 and 3, we have vendored the "six" package. You can read more about this and how to use it at https://pythonhosted.org/six/

Once the package supports Python 3 and the tests pass, we will also need to prepare for a new release. Bump the version number in `testing/mozbase/manifestparser/setup.py` to "2.0.0", and update the classifiers so they reflect the versions of Python that we now support.

Finally, to prepare a universal distribution, create a `testing/mozbase/manifestparser/setup.cfg` file with the following contents:

```
[bdist_wheel]
universal=1
```

When your patch(es) are ready, you will need to push them for review. We are use MozReview for this, and instructions for how to prepare your machine and how to structure your commit messages can be found in our guide: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html

There may be some review issues to address, but once your contribution has been approved and subsequently landed, we will release the new version of the package!

Comment 2

[Sandeep Bypina [:sandeepb]]

I am new to Mozilla and i have recently started contributing to it. I have the entire firefox environment set. I would like to work on this bug while using some mentoring. Please do let me know if i can start working on this..!

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Sandeep. Thank you for your interest! 

In regard of this bug no-one else mentioned to work on it yet. So feel free to get started. If you have problems please let us know.

Also please be aware that Dave is currently out. In the meantime I will try to get as best as possible answers to you if questions come up.

Comment 4

[Sandeep Bypina [:sandeepb]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Hi Sandeep. Thank you for your interest! 
> 
> In regard of this bug no-one else mentioned to work on it yet. So feel free
> to get started. If you have problems please let us know.
> 
> Also please be aware that Dave is currently out. In the meantime I will try
> to get as best as possible answers to you if questions come up.

Thank you henrik.  Will start working on this.

Comment 5

[Sandeep Bypina [:sandeepb]]

Attachment: Remaining test errors on python 3

Hi Henrik,

    I have made the changes for manifestparser to support both Python 2 & 3. All tests are passed in python 2.7. But for python 3.5, i am getting 3 failed tests which are attached as file. I was able to understand the reasons for the errors but am unable to resolve them. Could you please help me on resolving this errors.

As for the failed tests:

1. The "TypeError: 'NoneType' object is not callable" in filters.py is due to non-implementation of __hash__() in python 3 but i am unclear where and how to implement that in current code.

2. The "ValueError: min() arg is an empty sequence" in test_chunking.py might be due to `lengths` having no elements. But in that case, shouldn't max() also throw that error?

3. The " AssertionError: Lists differ: [] != ['test_3']" in test_manifestparser.py says that parser.get('name',x='level_3') is giving empty ([]) list. Does this have anything to do with my changes in manifestparser.py line 185 ? I changed that line to the following 
> data = dict(six.viewitems(self._ancestor_defaults) | six.viewitems(data))

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I haven't done that much with Python 3 yet. Given that Dave is on PTO I will forward your questions to Andrew who worked closely with Dave previously in getting the basics for Python 3 into the tree. I'm fairly sure that he can help you with those questions.

Comment 7

[Andrew Halberstadt [:ahal]]

Hi Sandeep, I don't know the answers to any of these questions off the top of my head. Could you upload what you have so far so I can try it out myself?

If the solution ends up being too hard we could skip these particular tests on python 3 and land what you have so far (then file a follow-up to fix up the rest).

Comment 8

[Andrew Halberstadt [:ahal]]

For your first issue, calling super(InstanceFilter, self).__eq__(other) instead of doing manually doing the __hash__() comparison might work.

Comment 9

[Sandeep Bypina [:sandeepb]]

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> For your first issue, calling super(InstanceFilter, self).__eq__(other)
> instead of doing manually doing the __hash__() comparison might work.

Andrew, this worked. Thanks.

I am trying to push the changeset to mozReview but it is saying "MozReview is no longer accepting new review requests". Please let me know how to proceed.

Comment 10

[Andrew Halberstadt [:ahal]]

Yes, we've switched over to phabricator now:
http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

For now you could just upload a diff of your patch to this bug (though you'll want to get phabricator set up anyway if you plan to be asking for review somewhat frequently).

Comment 11

[Sandeep Bypina [:sandeepb]]

Attachment: bug-1428705.patch

Andrew, i attached the latest diff file. Could you please check it and provide feedback.

Comment 12

[Andrew Halberstadt [:ahal]]

Comment on attachment 8999273 [details] [diff] [review]
bug-1428705.patch

Thanks!

The min error is due to changes in 'map' between python 2 and 3:
http://python-future.org/compatible_idioms.html#map (the option 1 solution is likely good enough here)

After fixing that I still see two other errors, though I can't seem to find any obvious reason for them to fail. The error in test_manifestparser.py seems to be intermittent, which is.. strange.

If you don't want to dig into these any further I'd suggest we skip these two remaining errors (via pytest.skip instead of the manifest.ini so we only skip those specific test functions). That way we can land what you have so far and prevent it from bit-rotting.

Comment 13

[Sandeep Bypina [:sandeepb]]

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Comment on attachment 8999273 [details] [diff] [review]
> bug-1428705.patch
> 
> Thanks!
> 
> The min error is due to changes in 'map' between python 2 and 3:
> http://python-future.org/compatible_idioms.html#map (the option 1 solution
> is likely good enough here)
> 
> After fixing that I still see two other errors, though I can't seem to find
> any obvious reason for them to fail. The error in test_manifestparser.py
> seems to be intermittent, which is.. strange.
> 
> If you don't want to dig into these any further I'd suggest we skip these
> two remaining errors (via pytest.skip instead of the manifest.ini so we only
> skip those specific test functions). That way we can land what you have so
> far and prevent it from bit-rotting.

Thanks Andrew. I fixed the min error. 

I want to dig into the remaining errors so that we can complete the porting of manifestparser. 

Regarding the error in test_manifestparser.py, I doubt that my changes in manifestparser.py might be affecting data we get. Could you please check that and let me know.

Comment 14

[Sandeep Bypina [:sandeepb]]

Attachment: Bug 1428705 - [manifestparser] Add support for Python 3. r=ahal

MozReview-Commit-ID: IGrh4OArTOu

Comment 15

[Andrew Halberstadt [:ahal]]

I don't believe the failure has anything to do with your changes in that file. As I mentioned, it's intermittent, so something much more tricky and subtle is likely happening.

I took another look at the failures in both test_chunking.py and test_manifestparser.py and I can't figure them out.

Comment 16

[Sandeep Bypina [:sandeepb]]

(In reply to Andrew Halberstadt [:ahal] from comment #15)
> I don't believe the failure has anything to do with your changes in that
> file. As I mentioned, it's intermittent, so something much more tricky and
> subtle is likely happening.
> 
> I took another look at the failures in both test_chunking.py and
> test_manifestparser.py and I can't figure them out.

Andrew, I made the changes suggested to the differential revision. 

Regarding the intermittent failures, i guess you are right. The mentioned errors are not getting shown on every test run.

Comment 17

[Sandeep Bypina [:sandeepb]]

Dave, I have made the final changes except the setup.cfg file. Could you please check those on phabricator and suggest how to proceed further?

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sorry for not clearing the needinfo. I've commented on the patch here https://phabricator.services.mozilla.com/D3287#120102

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Hey :sandeepb are you still interested in working on this patch?

Comment 20

[Sandeep Bypina [:sandeepb]]

Hi :davehunt, i am still interested in working on this patch. Been little busy with work.

Going by our last discussion, I have made one of the changes suggested as mentioned here https://phabricator.services.mozilla.com/D3287#166110

I will work on the mentioned reraise issue.

Regarding intermittent failures, i am still unable too find the reason for them to appear occasionally. Could you provide some more pointers to debug this.

Comment 21

[Sandeep Bypina [:sandeepb]]

(In reply to Dave Hunt [:davehunt] [he/him] ⌚️UTC from comment #19)

Hey :sandeepb are you still interested in working on this patch?

forgot to clear needinfo.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks :sandeepb, let us know when you have an update. Regarding the intermittent failures, if I recall correctly these were quite easy for my to reproduce locally just by running the tests multiple times. Are you able to replicate these failures?

Comment 23

[Sandeep Bypina [:sandeepb]]

(In reply to Dave Hunt [:davehunt] [he/him] ⌚️UTC from comment #22)

Thanks :sandeepb, let us know when you have an update. Regarding the intermittent failures, if I recall correctly these were quite easy for my to reproduce locally just by running the tests multiple times. Are you able to replicate these failures?

Hi :davehunt, I tried debugging the test_chunking.py for intermittent failures. One problem i identified is with chunk_by_round_robin method which is returning chunks list based on the runtime of last chunk.

Specifically in our case, for the 2nd test we have 4 directories which generates only 3 tests because of directory bar with 0 value. These 3 tests have runtimes of (3,14,81). In chunk_by_round_robin, we are creating manifest set(unordered collection). If the chunk of runtime 3 is processed last, we get only 3 chunks(scenario 1). In other cases, we get 14 and 81 chunks of which only 3 chunks are of tests and remaining are empty lists(scenario 2).

While creating chunks normally, we create an empty chunk for bar as well. Because of this, the runtime_delta of local chunks is 81-0 every time. But when round robin returns only 3 chunks(scenario 1), the runtime_delta of those chunks is 81-3=78 which is less than our normal runtime_delta and thus the test failure.

To rectify this, I want to create max(runtime) of chunks rather than the runtime of last test in chunk_by_round_robin method. Could you please let me know whether that is an appropriate fix or not?

Comment 24

[Sandeep Bypina [:sandeepb]]

(In reply to Dave Hunt [:davehunt] [he/him] ⌚️UTC from comment #22)

Thanks :sandeepb, let us know when you have an update. Regarding the intermittent failures, if I recall correctly these were quite easy for my to reproduce locally just by running the tests multiple times. Are you able to replicate these failures?

Hi :davehunt, Please check my earlier comment and suggest a possible solution.

Regarding the second intermittent failure occuring in test_manifestparser.py, I found the reason for it to happen. But i cant pinpoint why it is happening intermittently.
In this case, we are creating ManifestParser Object in test_parent_defaults method in test_manifestparser.py. The parser data is different when the tests are failing. I couldnt identify why the parser object is different on different runs.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Sandeep Bypina [:sandeepb] from comment #23)

(In reply to Dave Hunt [:davehunt] [he/him] ⌚️UTC from comment #22)

Thanks :sandeepb, let us know when you have an update. Regarding the intermittent failures, if I recall correctly these were quite easy for my to reproduce locally just by running the tests multiple times. Are you able to replicate these failures?

Hi :davehunt, I tried debugging the test_chunking.py for intermittent failures. One problem i identified is with chunk_by_round_robin method which is returning chunks list based on the runtime of last chunk.

Specifically in our case, for the 2nd test we have 4 directories which generates only 3 tests because of directory bar with 0 value. These 3 tests have runtimes of (3,14,81). In chunk_by_round_robin, we are creating manifest set(unordered collection). If the chunk of runtime 3 is processed last, we get only 3 chunks(scenario 1). In other cases, we get 14 and 81 chunks of which only 3 chunks are of tests and remaining are empty lists(scenario 2).

While creating chunks normally, we create an empty chunk for bar as well. Because of this, the runtime_delta of local chunks is 81-0 every time. But when round robin returns only 3 chunks(scenario 1), the runtime_delta of those chunks is 81-3=78 which is less than our normal runtime_delta and thus the test failure.

To rectify this, I want to create max(runtime) of chunks rather than the runtime of last test in chunk_by_round_robin method. Could you please let me know whether that is an appropriate fix or not?

I'm not familiar with this code, perhaps :ahal can help here?

Comment 26

[Andrew Halberstadt [:ahal]]

Hi Sandeep, I think the fix sounds good but I'm having a bit of trouble following the problem. Specifically what is different between python 2/3 that would cause this intermittent?

Could you push your fix to phabricator and flag me for review? I think it would help to see what you are talking about in the code. Thanks!

Comment 27

[Sandeep Bypina [:sandeepb]]

(In reply to Andrew Halberstadt [:ahal] from comment #26)

Hi Sandeep, I think the fix sounds good but I'm having a bit of trouble following the problem. Specifically what is different between python 2/3 that would cause this intermittent?

Could you push your fix to phabricator and flag me for review? I think it would help to see what you are talking about in the code. Thanks!

Hi Andrew, I have pushed the fix to phabricator and flagged you. Please take a look at the code and suggest any changes needed.
Thanks.

Comment 28

[Andrew Halberstadt [:ahal]]

Nice investigation! That makes sense, I can see by looking at the old code why this might fail intermittently.

So it's probably blind luck that the python 2 set implementation was iterating over the items in the set consistently. The python 3 implementation must be more likely to iterate in a different order.

I set :davehunt as a blocking reviewer to make sure he's happy with the other changes before proceeding. Thanks very much for figuring this out :)

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

championshuttler: are you working on this now?

Comment 30

[Shivam Singhal [ :championshuttler ]]

Hi Dave, really sorry for the late reply I updated the patch on behalf of Sandeep and later :ahal and me were testing it but its like sometimes the tests are passing and sometimes its not, even sometimes tests are failing for python 2.7 , we did not get any proper reason behind it thats why we had to leave it for future.

@ahal can you please provide more info if you remember something about it.

Thanks

Comment 31

[Andrew Halberstadt [:ahal]]

Sorry, I don't remember what the issue was beyond my comment here:
https://phabricator.services.mozilla.com/D3287#1408363

IIRC Shivam only fixed some lint issues so that we could land the patch (at which point I discovered the test failures). Sandeep, were you still interested in investigating this?

Comment 32

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1428705 - [manifestparser] Add support for Python 3. r=egao

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f85d054c6a1b
[manifestparser] Add support for Python 3. r=egao

Comment 34

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f85d054c6a1b

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Edwin, are you going to push the changes to PyPI?

Comment 36

[Edwin Takahashi (:egao | infrequent contributor)]

Looks like the patch landed successfully.

Next task is to ensure the remaining two tests from https://searchfox.org/mozilla-central/source/testing/mozbase/manifestparser/tests/manifest.ini enabled to run with python3 as well as python2.

Other remaining tasks is to do a pass over all files once the above is complete, to ensure that all problematic sections of code have been addressed to be python2/3 safe. Use 2to3 as reference if unsure.

As a penultimate item - general code style cleanup by applying pep8 and other linters.

Lastly, prior to closing this bug, do a major version bump to indicate some significant work has taken place, and upload to pypi (which I can handle, assuming I have the permission).

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh I see. More changes are necessary.

Comment 38

[Edwin Takahashi (:egao | infrequent contributor)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #35)

Edwin, are you going to push the changes to PyPI?

I could publish it now, though two tests are still failing on python3 so has been temporarily disabled. I wouldn't consider the work to be done at this state; I'm hoping that we'll be able close this bug out with all work done in a week or two, possibly faster.

Thanks for following up though!

Comment 39

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1428705 - fix manifestparser/test_chunking.py for python3

Comment 40

[Edwin Takahashi (:egao | infrequent contributor)]

This latest patch should fix issues with test_chunking.py, leaving only the test_directory_covert.py as the outstanding item.

Comment 41

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1428705 - fix manifestparser/manifestparser for python3 and enable test_convert_directory.py

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ffd4ecbfc2b
fix manifestparser/manifestparser for python3 and enable test_convert_directory.py r=ahal

Comment 43

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9ffd4ecbfc2b

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb6c737e982a
fix manifestparser/test_chunking.py for python3 r=ahal,whimboo

Comment 45

[Edwin Takahashi (:egao | infrequent contributor)]

Once the above patch lands on m-c I believe we are done with the python3 compatibility migration. If so, I will release a new version to pypi with major version bump.

Comment 46

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/bb6c737e982a

Comment 47

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1428705 - use assertEqual in test_manifest.py and add formal python3 compatibility in setup.py for manifestparser

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55e7b7f75e47
use assertEqual in test_manifest.py and add formal python3 compatibility in setup.py for manifestparser r=Callek

Comment 49

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/55e7b7f75e47

Comment 50

[Shivam Singhal [ :championshuttler ]]

Attachment: Bug 1428705 - Fix the manifest file for manifestparser.r=egao

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4be58aaa571c
Fix the manifest file for manifestparser.r=egao

Comment 52

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4be58aaa571c