[MRG] Working towards python3 compatible codebase #109

manu-chroma · 2017-05-22T16:03:00Z

Related issue checklist: #108

I'd prefer if the commits from this branch are preserved when merging into the master. This will ensure that future potential issues are easier to bisect. That's why I've ensured that the commit history is clean and commit messages are informative.

tetron · 2017-05-22T19:06:02Z

What happens if you add from __future__ import unicode_literalso that it uses unicode strings consistently?

mr-c · 2017-05-22T19:15:56Z

looking good!

manu-chroma · 2017-05-22T19:18:44Z

@tetron It allows in a Python 2 program to have the default interpretation of string literals be Unicode (UTF8). That is,

from __future__ import unicode_literals
 
s = "my surname is Häggholm"

is equivalent to

s = u"my surname is Häggholm"

except that I don’t have to prefix every single string with u to treat it as a Unicode literal.

Looks good to me, but it seems to have drawbacks too as highlighted in this article: http://python-future.org/unicode_literals.html
I am not very familiar with the codebase so I'd be interested in your opinion on this. Currently reading more sources and having look at other projects which are py2+3 compatible to get more idea.

manu-chroma · 2017-05-22T19:28:20Z

@tetron @mr-c feel free to drop comments on individual commits. I'll do some cleanup and testing for corner cases now.

tetron · 2017-05-22T19:34:52Z

Thanks for the link.

If possible, I'd prefer to use Unicode strings consistently. Some string literals are already marked with u"" but doing that everywhere is tedious. The most likely problem is calling Python2 APIs which expect native strings and break if passed unicode. I think it is at least worth a try to find out what breaks.

The main client of schema-salad is cwltool. Current cwltool has a version restriction of < 3 so we should be able to safely release a 3.0 schema-salad package without breaking existing cwltool installations.

manu-chroma · 2017-05-22T19:50:08Z

Thanks for the heads up.

The most likely problem is calling Python2 APIs which expect native strings and break if passed unicode.

Yes, this is my main concern too.

I think it is at least worth a try to find out what breaks.

Sure, I will try this approach. Interested to see how this works out.

I think it would be better to get this PR merged before getting on with it. Would keep things simpler

tetron · 2017-05-23T13:29:32Z

Here's how I would like to do this. The goal is minimize the number of merges & schema-salad releases in the event there are issues in schema salad that turn up while porting cwltool.

in this branch setup.py, change version="3.0"
in cwltool, create a new python3 branch and change the dependency to schema-salad >= 3.0
for development, create a virtualenv and clone/install python3 branches for both schema-salad and cwltool
fix up cwltool for python3 support & pass conformance tests using both python 2.7 and 3.4
merge & release schema-salad 3.0
this should make it possible for the cwltool PR tests to pass
once those are green, merge & release cwltool

manu-chroma · 2017-05-23T15:09:57Z

I like the overall workflow. This would give me a chance to see how changes in schema_salad are breaking conformance tests early on.
One thing I will have to take care about is the merge conflicts that will arise since @kapilkd13 will also be working on both the repos for MS Windows feature.

kapilkd13 · 2017-05-23T18:51:45Z

Nice 😃

AleksandrSl · 2017-05-27T09:57:42Z

About typesheds for python3, there are already many of them in python typeshed repo https://github.com/AleksandrSl/schema_salad/blob/python3-support/typeshed.md. I have raised issues about typeshed addition about two months ago in mistune in cachecontrol repos, but got no answer so far, so I think I will try to write them myself. Is there any modules that I've not mentioned or haven't find ready typesheds for?

manu-chroma · 2017-05-27T20:29:56Z

Updates:

Enabled Python 3.x (x = 3,4,5,6) on Travis for unit and lint tests.
Tested this branch of schema_salad with current master of cwltool locally. Unit tests and conformance tests passing. Thus, changes made so far don't break backward compatibility with Python 2.7. 😅

manu-chroma · 2017-05-27T20:38:59Z

@AleksandrSl Thanks so much for compiling this. Before running mypy in Python 3 mode I've plans to bump mypy to latest version and then try to make it run in Python 3 and specifically aim for --strict flag. 😄 Switching on --strict flag definately requires better annotated stubs files/codebase AFAIK.

I will go through your typshed list and try to update it for the missing fields. Also, we've used stubgen for some of the 3rd party dependencies so far. So, need to decide if we will continue with that or write our own stubs for some of them.

anton-khodak · 2017-06-02T06:57:16Z

schema_salad/tests/test_examples.py

@@ -299,7 +301,7 @@ def test_scoped_id(self):
                          }, ra)

        g = makerdf(None, ra, ctx)
-        print(g.serialize(format="n3"))
+        print((g.serialize(format="n3")))


Why do we wrap these expressions in additional parentheses? Just wondering

this change was done by modernize fixture. I'll fix it.

anton-khodak · 2017-06-02T07:11:02Z

tox.ini

 deps =
-  mypy==0.470
-  typed-ast==0.6.3
  -rrequirements.txt


six requirement missing in requirements.txt

Earlier the tests were passing because six was being satisfied as a requirement of some other package. I will add it as an explicit dependency now.
I believe I should add in setup.py rather than requirements.txt because it is required beyond testing.

AleksandrSl · 2017-06-02T22:18:09Z

schema_salad/makedoc.py

@@ -227,7 +234,7 @@ def typefmt(self,
                nbsp=False,             # type: bool
                jsonldPredicate=None    # type: Optional[Dict[str, str]]
                ):
-        # type: (...) -> Union[str, unicode]
+        # type: (...) -> Union[str, Text]


Isn't Text an alias for str in python3 and unicode in python2, why mention str separately?

Separately mentioning is redundant in python3 indeed, but in case of python2 union states that this function can return ASCII strings as well as unicode strings.
# type: (...) -> Union[str, unicode] annotation was already present, I just made sure Text type makes it compatible with python3.

@manu-chroma It seems, though, that simple Text is enough.
"When checking Python 2.7 code, type checkers should treat the int and long types as equivalent. For parameters typed as unicode or Text , arguments of type str should be acceptable." from PEP484
I checked this with mypy, str and unicode pass for Text in python2 and str for Text in python3

@AleksandrSl Thanks for the clarifying this. 😄

I will make changes wherever required in PR #111

Main changes: - use of __future__ - using six lib to: - deal with checking unicode - dict operations - other API changes

manu-chroma · 2017-06-27T17:02:52Z

In the process of rebasing and resolving merge conflicts, I ended up having some commits messages appearing twice. Just messed up commit history a little. 😓

edit: around 3-4 commits. shouldn't be a problem

manu-chroma · 2017-06-27T17:22:13Z

schema_salad/sourceline.py

            _add_lc_filename(d, source)

-def relname(source):  # type: (AnyStr) -> AnyStr


Any particular reason this was marked as AnyStr? I don't expect this to take binary strings.

No reason I can think of

This was before Text was a thing

alright. 👍

manu-chroma · 2017-06-27T17:22:40Z

schema_salad/sourceline.py

    _add_lc_filename(r, relname(source))

-def reflow(text, maxline, shift=""):  # type: (AnyStr, int, AnyStr) -> AnyStr


query same as above.

manu-chroma · 2017-06-27T17:40:00Z

This branch is pretty mature now. I would like to get it reviewed and merged now.
cc: @mr-c @tetron @anton-khodak

mr-c · 2017-06-28T09:49:26Z

setup.py


-install_requires.append("avro")  # TODO: remove me once cwltool is
+# install_requires.append("avro")  # TODO: remove me once cwltool is
 # available in Debian Stable, Ubuntu 12.04 LTS


these lines can now be removed

mr-c · 2017-06-28T09:50:12Z

setup.py


 setup(name='schema-salad',
-      version='2.5',
+      version='3.0',


Is the API really different? Does the current released version ofcwltool still pass its tests and the CWL conformance tests which this branch? If not, then 2.6 or 2.5.1 is fine.

This version bump was motivated by @tetron's comment.
#109 (comment)

in the event there are issues in schema salad that turn up while porting cwltool.

So let's test that as I mentioned in my previous comment

Does the current released version ofcwltool still pass its tests and the CWL conformance tests which this branch?

conformace and unit tests pass

mypy fails. https://travis-ci.org/manu-chroma/cwltool/jobs/247893307#L194

this is due to changes made when upgrading mypy to 0.511 for schema_salad. But cwltool master is still uses mypy==0.470.

we can selectively silence errors due to schema_salad using mypy config file until mypy==0.511 changes are not merged in cwltool.

That's fine, as long as we fix it this week.

yeah. I'm preparing cwltool PR to get reviewed soon.

mr-c · 2017-06-28T09:50:18Z

setup.py

@@ -67,7 +69,7 @@
          'console_scripts': ["schema-salad-tool=schema_salad.main:main", "schema-salad-doc=schema_salad.makedoc:main"]
      },
      zip_safe=True,
-      cmdclass={'egg_info': tagger},
+      # cmdclass={'egg_info': tagger},


i've fixed it.
when testing with cwltool earlier, it was giving me errors related to requirement parsing. thought commenting this out might help. forgot to uncomment.

mr-c · 2017-06-28T11:51:40Z

setup.py


 setup(name='schema-salad',
-      version='2.5',
+      version='3.0',


Change this to 2.5.1 to 2.6 and I'll merge it!

2.5.1 or 2.6?

I think 2.5.1 is fine.

mr-c · 2017-06-28T12:15:37Z

Huzzah! Thank you @manu-chroma

manu-chroma · 2017-06-28T12:18:26Z

thank you @mr-c 😅

manu-chroma · 2017-06-28T12:22:03Z

next PR for schema_salad will be doing unicode_literals import everywhere. will get cwltool PR merged before proceeding with this.

manu-chroma changed the title ~~Working towards python3 compatible codebase~~ [WIP] Working towards python3 compatible codebase May 22, 2017

manu-chroma force-pushed the modernize branch from 6065160 to 4bdfe72 Compare May 22, 2017 19:10

tetron mentioned this pull request May 23, 2017

path to full python2+python3 support common-workflow-language/cwltool#310

Closed

6 tasks

manu-chroma force-pushed the modernize branch 2 times, most recently from aa13415 to 8c17480 Compare May 27, 2017 20:24

manu-chroma force-pushed the modernize branch 3 times, most recently from 05fdce9 to b034a36 Compare May 30, 2017 10:31

manu-chroma mentioned this pull request May 30, 2017

Support Python 3 common-workflow-language/cwltool#8

Closed

anton-khodak reviewed Jun 2, 2017

View reviewed changes

manu-chroma force-pushed the modernize branch from 6054dc3 to fe06748 Compare June 2, 2017 07:31

AleksandrSl reviewed Jun 2, 2017

View reviewed changes

manu-chroma force-pushed the modernize branch from e537077 to b646e8b Compare June 6, 2017 06:39

manu-chroma force-pushed the modernize branch from b646e8b to 8155231 Compare June 27, 2017 13:10

manu-chroma added 3 commits June 27, 2017 18:41

Apply Python3 modernize transforms

3080c69

Main changes: - use of __future__ - using six lib to: - deal with checking unicode - dict operations - other API changes

mypy type annotations fix: replace unicode -> Text

e9bb3ec

modernize tests

df685ea

manu-chroma added 2 commits June 27, 2017 22:25

mypy_requirements.txt: bump mypy version

3d60033

bump ruamel version, put upper bound on ruamel in setup.py

b2a5a26

manu-chroma commented Jun 27, 2017

View reviewed changes

manu-chroma changed the title ~~[WIP] Working towards python3 compatible codebase~~ [MRG] Working towards python3 compatible codebase Jun 27, 2017

schema.py: fix some types, remove redundant str

7748526

manu-chroma mentioned this pull request Jun 27, 2017

[WIP] mypy updates #111

Closed

manu-chroma requested review from tetron, mr-c and anton-khodak June 27, 2017 18:00

mr-c reviewed Jun 28, 2017

View reviewed changes

setup.py: cleanup

d9bdc8b

manu-chroma force-pushed the modernize branch from ad396b9 to d9bdc8b Compare June 28, 2017 09:59

manu-chroma added a commit to manu-chroma/cwltool that referenced this pull request Jun 28, 2017

use schema_salad from common-workflow-language/schema_salad#109

3345141

manu-chroma added a commit to manu-chroma/cwltool that referenced this pull request Jun 28, 2017

use schema_salad from common-workflow-language/schema_salad#109

2335350

manu-chroma added a commit to manu-chroma/cwltool that referenced this pull request Jun 28, 2017

use schema_salad from common-workflow-language/schema_salad#109

6c35577

mr-c self-requested a review June 28, 2017 11:50

mr-c approved these changes Jun 28, 2017

View reviewed changes

mr-c requested changes Jun 28, 2017

View reviewed changes

manu-chroma and others added 2 commits June 28, 2017 17:26

setup.py: change version, uncomment classifiers for py3

a8903de

3.6 too!

52c054e

mr-c approved these changes Jun 28, 2017

View reviewed changes

mr-c merged commit fa93acc into common-workflow-language:master Jun 28, 2017

		_add_lc_filename(d, source)

		def relname(source): # type: (AnyStr) -> AnyStr

		_add_lc_filename(r, relname(source))

		def reflow(text, maxline, shift=""): # type: (AnyStr, int, AnyStr) -> AnyStr

[MRG] Working towards python3 compatible codebase #109

[MRG] Working towards python3 compatible codebase #109

Conversation

manu-chroma commented May 22, 2017 • edited

tetron commented May 22, 2017

mr-c commented May 22, 2017

manu-chroma commented May 22, 2017 • edited

manu-chroma commented May 22, 2017

tetron commented May 22, 2017 • edited

manu-chroma commented May 22, 2017

tetron commented May 23, 2017

manu-chroma commented May 23, 2017

kapilkd13 commented May 23, 2017

AleksandrSl commented May 27, 2017

manu-chroma commented May 27, 2017 • edited

manu-chroma commented May 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anton-khodak Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma commented Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma commented Jun 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma Jun 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manu-chroma Jun 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-c commented Jun 28, 2017

manu-chroma commented Jun 28, 2017

manu-chroma commented Jun 28, 2017

manu-chroma commented May 22, 2017 •

edited

manu-chroma commented May 22, 2017 •

edited

tetron commented May 22, 2017 •

edited

manu-chroma commented May 27, 2017 •

edited

anton-khodak Jun 2, 2017 •

edited

manu-chroma commented Jun 27, 2017 •

edited

manu-chroma commented Jun 27, 2017 •

edited

manu-chroma Jun 28, 2017 •

edited

manu-chroma Jun 28, 2017 •

edited