Add Test Cases for Python 3.7 #720

seii-saintway · 2019-10-04T03:23:17Z

googlebot · 2019-10-04T03:23:31Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

seii-saintway · 2019-10-04T03:25:53Z

@googlebot I signed it!

googlebot · 2019-10-04T03:25:55Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

coveralls · 2019-10-04T03:38:12Z

Coverage decreased (-0.01%) to 77.86% when pulling 7934bcf on seii-saintway:fix/py37-async-and-yaml into 8c2df84 on googledatalab:master.

paveldournov

LGTM

crwilcox · 2020-03-05T23:28:22Z

This seems good, except for some linting errors:

./datalab/utils/_async.py:66:101: E501 line too long (103 > 100 characters)
./google/datalab/utils/_async.py:66:101: E501 line too long (103 > 100 characters)

crwilcox · 2020-03-10T21:08:58Z

Thanks @seii-saintway for this. Due to the linting issues and an atypical var name, I ended up making a new PR, #728 that does a very similar thing. I also didn't include yaml loader changes. I am not sure how those relate to python3.7+ support.

seii-saintway · 2020-03-10T23:38:17Z

Thanks @crwilcox for the new PR. I have some memory of troubles of the yaml loader thing. And I think we should consider thoroughly. Because I felt that there are some other troubles in Python 3.7 environment.

googlebot · 2020-03-11T05:32:24Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

seii-saintway · 2020-03-11T05:36:38Z

I just want to see if I can merge this PR or not after I solve the linting errors
(^_^)

seii-saintway · 2020-03-11T05:44:04Z

@googlebot I signed it!

seii-saintway · 2020-03-11T06:13:16Z

@googlebot I fixed it.

seii-saintway · 2020-03-11T06:44:07Z

@googlebot I fixed it.

googlebot · 2020-03-11T06:44:11Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

monotaro-sheng-wei · 2020-03-11T06:53:53Z

@googlebot I consent.

googlebot · 2020-03-11T06:53:58Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

crwilcox · 2020-03-11T17:14:26Z

google/datalab/utils/commands/_utils.py

@@ -328,6 +329,8 @@ def parse_config(config, env, as_dict=True):
    config = {}
  elif stripped[0] == '{':
    config = json.loads(config)
+  elif six.PY3:
+    config = yaml.load(config, Loader=yaml.FullLoader)


What are these loader changes addressing? This seems unrelated to Python 3.7 compat to me at first glance.

I am sorry that I cannot remember the reason clearly.
But I searched on google and found an issue concerning pyyaml version.

https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation

Why does PyYAML 5.1 raise YAMLLoadWarning when the default loader has been made safer already? yaml/pyyaml#292

I remember that I built the datalab image with py37 and got into some troubles with pyyaml before.

As the pyyaml version show as below,

pydatalab/setup.py

Line 108 in 8c2df84

'pyyaml>=3.11',

I guess that pyyaml will be installed as different version in py2 and py3.

As you could see in the py35 coverage test, there were warnings concerning YAMLLoadWarning.

/home/travis/build/googledatalab/pydatalab/tests/pipeline/pipeline_tests.py:305: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details. dag_dict = yaml.load(PipelineTest._test_pipeline_yaml_spec)

/home/travis/build/googledatalab/pydatalab/tests/pipeline/pipeline_tests.py:265: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details. dag_dict = yaml.load(PipelineTest._test_pipeline_yaml_spec)

/home/travis/build/googledatalab/pydatalab/tests/pipeline/pipeline_tests.py:280: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details. dag_dict = yaml.load(PipelineTest._test_pipeline_yaml_spec)

Maybe there will be warnings or errors in py37, too.
I'm sorry that maybe it is better to open a new PR concerning the YAMLLoadWarning thing.
And it is better add some py37 coverage tests.

crwilcox · 2020-03-11T17:15:44Z

datalab/utils/__init__.py

@@ -12,7 +12,7 @@

 """Google Cloud Platform library - Internal Helpers."""

-from ._async import async, async_function, async_method
+from ._async import async_, async_function, async_method


Thanks for incorporating the naming style from #728. It is shorter and a more standard pattern.

Thanks you very much for your support.

Add trove for py3

googledatalab#711 Fix Incompatibility with Python 3.7

55b4a07

paveldournov previously approved these changes Mar 5, 2020

View reviewed changes

yixinshi previously approved these changes Mar 6, 2020

View reviewed changes

fix some linting errors

7c46758

seii-saintway dismissed stale reviews from yixinshi and paveldournov via 7c46758 March 11, 2020 05:32

seii-saintway requested review from paveldournov and yixinshi March 11, 2020 05:33

monotaro-sheng-wei added 2 commits March 11, 2020 14:53

rollback my mistake

f453d80

try to fix incompatibility between py2 and py3

2fabaa7

crwilcox reviewed Mar 11, 2020

View reviewed changes

monotaro-sheng-wei added 3 commits March 13, 2020 15:44

Add Loader=yaml.FullLoader

4eb5043

Add trove for py3

Add py37 for travis

5f0bfde

Add py37 for travis

43a6a3e

monotaro-sheng-wei added 3 commits March 17, 2020 09:57

Add py37 for travis

391ba87

Add py37 for travis

c1ea339

Add py37 for travis

7934bcf

seii-saintway changed the title ~~#711 Fix Incompatibility with Python 3.7~~ Add Test Cases for Python 3.7 Mar 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Test Cases for Python 3.7 #720

Add Test Cases for Python 3.7 #720

seii-saintway commented Oct 4, 2019

googlebot commented Oct 4, 2019

seii-saintway commented Oct 4, 2019

googlebot commented Oct 4, 2019

coveralls commented Oct 4, 2019 •

edited

Loading

paveldournov left a comment

crwilcox commented Mar 5, 2020

crwilcox commented Mar 10, 2020 •

edited

Loading

seii-saintway commented Mar 10, 2020 •

edited

Loading

googlebot commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

googlebot commented Mar 11, 2020

monotaro-sheng-wei commented Mar 11, 2020

googlebot commented Mar 11, 2020

crwilcox Mar 11, 2020

seii-saintway Mar 12, 2020 •

edited

Loading

crwilcox Mar 11, 2020

seii-saintway Mar 12, 2020

Add Test Cases for Python 3.7 #720

Are you sure you want to change the base?

Add Test Cases for Python 3.7 #720

Conversation

seii-saintway commented Oct 4, 2019

googlebot commented Oct 4, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

seii-saintway commented Oct 4, 2019

googlebot commented Oct 4, 2019

coveralls commented Oct 4, 2019 • edited Loading

paveldournov left a comment

Choose a reason for hiding this comment

crwilcox commented Mar 5, 2020

crwilcox commented Mar 10, 2020 • edited Loading

seii-saintway commented Mar 10, 2020 • edited Loading

googlebot commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

seii-saintway commented Mar 11, 2020

googlebot commented Mar 11, 2020

monotaro-sheng-wei commented Mar 11, 2020

googlebot commented Mar 11, 2020

crwilcox Mar 11, 2020

Choose a reason for hiding this comment

seii-saintway Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

crwilcox Mar 11, 2020

Choose a reason for hiding this comment

seii-saintway Mar 12, 2020

Choose a reason for hiding this comment

coveralls commented Oct 4, 2019 •

edited

Loading

crwilcox commented Mar 10, 2020 •

edited

Loading

seii-saintway commented Mar 10, 2020 •

edited

Loading

seii-saintway Mar 12, 2020 •

edited

Loading