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 to Marshmallow 3 #57

Open
wants to merge 8 commits into
base: v1.8
Choose a base branch
from
Open

Conversation

aksswami
Copy link

Migration to marshmallow 3, specifically 3.6.1
Also removed python27 env from tox.

I am able to run all test successfully except below two test related to Oozie workflow. As I have very little understanding of Oozie workflow, not able to resolve these two. Any help would be really appreciated.

test/oozier/test_oozie_converter.py::test_workflow_parser FAILED
test/oozier/test_oozie_converter.py::test_workflow_parser_with_prune_forks FAILED

As boundary-layer master is for Python 2.7, and if you don't have plan to move to Python 3 as of now maybe we can create another branch for python3 changes and merge these changes to that.

@mchalek
Copy link
Member

mchalek commented Jun 17, 2020

wow, thanks @aksswami for this contribution! I will try to get around to taking a look at it within the next few days. Just wanted to let you know we haven't missed and/or forgotten about this :)

@aksswami
Copy link
Author

Great! It will be good to update the dependencies to latest stable. Let me know if there any other. task, I can help with.

@mchalek
Copy link
Member

mchalek commented Jun 25, 2020

OK @aksswami I finally got around to looking at this 😅 . The Oozie-related test errors you experienced were due to the fact that marshmallow 2.x permits unrecognized fields by default, whereas 3.x rejects unrecognized fields by default.

If you add the following to the OozieBaseSchema class, it should fix the tests:

    class Meta:
        unknown = ma.INCLUDE

I think it's a great idea to upgrade to ma3 and I really appreciate you doing all of the heavy lifting here! Personally, I am fine with cutting off python 2 support --- we can start a new minor version for this, and we can always back-port any additional fixes as necessary to the 1.7 trunk, which we can keep around on a separate branch somewhere. But as you know there is at least a little risk in doing that, and in addition, with these updates there are some other code cleanups we can do: for example, we can eliminate the StrictSchema which we use as the base class for the non-oozie-related portions of the code, because this is now the default in marshmallow. We can presumably also address the deprecation warnings around the use of abc, and other python3-specific improvements.

So, how about this: If you could make the change above so that the tests pass, we can then merge this PR into a branch to represent the start of boundary-layer version 1.8. Then maybe we can do any other cleanup we would like to do, we can verify the new version internally at Etsy against more workflows (to increase chances of hitting edge cases or whatever), and cut the 1.8.0 release sometime soon?

@aksswami
Copy link
Author

aksswami commented Jul 7, 2020

@mchalek Thanks for detailed explanation. I will cleanup this PR for Ozzie related error. And will try to create a new PR for other changes.

Yes, moving to Python3 will require more changes. So this PR can be start of those changes. Thanks again.

@aksswami
Copy link
Author

aksswami commented Jul 7, 2020

Resolved the OzzieBaseSchema test issue.

For reference:- https://marshmallow.readthedocs.io/en/3.0/quickstart.html#handling-unknown-fields

@aksswami
Copy link
Author

Do we need to disable 2.7 CI test? Can you help me resolve the failing checks? @mchalek

@mchalek
Copy link
Member

mchalek commented Jul 20, 2020

Thanks @aksswami for making the requested changes! Yes, let's disable the 2.7 check. You should be able to do that by editing line 16 of the file .github/workflows/python-app.yml. If you wouldn't mind doing that, hopefully that's all you'll need! I will change this PR now to target a new branch for boundary-layer v1.8.

@mchalek mchalek changed the base branch from master to v1.8 July 20, 2020 20:18
@aksswami
Copy link
Author

@mchalek Removed python 2.7 run from the test pipeline workflow.

@aksswami
Copy link
Author

@mchalek Is there anything else required for this PR to be merge ready? Please let me know.

@dossett
Copy link
Contributor

dossett commented Jul 28, 2020

Thanks @aksswami! Kevin and I talked offline and I will merge this PR soon (perhaps directly to the main branch) based on how some final py2 -> py3 migration work goes internally for us.

Thanks so much for this contribution!

@kconvey
Copy link

kconvey commented Sep 3, 2020

Is it possible to know what the status is of this PR?

@dossett
Copy link
Contributor

dossett commented Sep 14, 2020

Hi @kconvey (and everyone else following this PR). The good news is that we (Etsy) have done everything we need to do to cut loose of python2. The bad news is that my kids are back in school (distance learning in my city) so my capacity to drive some work like this is pretty limited. I hope to be able to work on it in the next couple of weeks. (The work that's outside of this PR is to make sure that we can upgrade marshmallow the same way in some internal codebases that would otherwise conflict with this change.)

@kconvey
Copy link

kconvey commented Sep 15, 2020

Thanks for the update! Glad to hear that this hasn't been forgotten about :)

@hanli-0612
Copy link

Is it possible to get some updates on this PR? It'd be great if we could support marshmallow 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants