Story #14965

Port arv-mount to Python 3

Added by Tom Morris 4 months ago. Updated about 2 hours ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/16/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0
Release relationship:
Auto

Subtasks

Task #15328: Review 14965-arv-mount-py-threeResolvedLucas Di Pentima


Related issues

Related to Arvados - Story #14532: [Epic] Port to Python 3 to prepare for Python 2 sunsetting in December 2019In Progress

Has duplicate Arvados - Bug #15251: arv-mount supports python 3Duplicate

Associated revisions

Revision caeb9d74
Added by Eric Biagiotti about 1 month ago

Merge remote-tracking branch 'origin/14965-arv-mount-py-three' into 14965-arv-mount-py-three

refs #14965
14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision b629d9e8
Added by Eric Biagiotti 23 days ago

Merge remote-tracking branch 'origin/master' into 14965-arv-mount-py-three

refs #14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision 76530546
Added by Eric Biagiotti 3 days ago

Merge branch 'master' into 14965-arv-mount-py-three

refs #14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision 16d9c611
Added by Eric Biagiotti 3 days ago

Merge branch '14965-arv-mount-py-three' of git.curoverse.com:arvados into 14965-arv-mount-py-three

refs #14965
14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision 05c87aaf
Added by Eric Biagiotti 2 days ago

Merge branch 'master' into 14965-arv-mount-py-three

refs #14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

Revision c1cf71ba
Added by Eric Biagiotti about 3 hours ago

Merge branch '14965-arv-mount-py-three'

refs #14965

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <>

History

#1 Updated by Tom Morris about 2 months ago

  • Parent task deleted (#14532)

#2 Updated by Tom Morris about 2 months ago

  • Target version changed from Arvados Future Sprints to To Be Groomed
  • Tracker changed from Task to Story

#3 Updated by Tom Morris about 2 months ago

  • Related to Story #14532: [Epic] Port to Python 3 to prepare for Python 2 sunsetting in December 2019 added

#4 Updated by Tom Morris about 2 months ago

  • Has duplicate Bug #15251: arv-mount supports python 3 added

#5 Updated by Tom Morris about 2 months ago

  • Story points set to 2.0
  • Target version changed from To Be Groomed to Arvados Future Sprints

#6 Updated by Eric Biagiotti about 1 month ago

  • Target version changed from Arvados Future Sprints to 2019-06-19 Sprint
  • Assigned To set to Eric Biagiotti

#7 Updated by Ward Vandewege about 1 month ago

  • Release set to 22

#8 Updated by Eric Biagiotti about 1 month ago

  • Status changed from New to In Progress

#9 Updated by Eric Biagiotti about 1 month ago

  • Target version changed from 2019-06-19 Sprint to 2019-07-03 Sprint

#10 Updated by Eric Biagiotti 16 days ago

  • Target version changed from 2019-07-03 Sprint to 2019-07-17 Sprint

#11 Updated by Eric Biagiotti 3 days ago

Latest at 16d9c611d5026e50692e1b3d0fefb951de5afa7b

  • arv-mount supports Python 3
  • Besides the futurize changes, I mostly fixed test encoding issues. I tried to minimize changes to our actual fuse code.

Testing:

#12 Updated by Eric Biagiotti 2 days ago

  • Target version changed from 2019-07-17 Sprint to Arvados Future Sprints

#13 Updated by Eric Biagiotti 2 days ago

  • Target version changed from Arvados Future Sprints to 2019-07-31 Sprint

#14 Updated by Lucas Di Pentima 2 days ago

Sorry for the delay, I was doing some manual testing on arvbox and it took me some time.

As we have had performance issues in the past with the fuse driver, some of my comments are headed towards that, following this Python 2/3 "cheat sheet": http://python-future.org/compatible_idioms.html

  • When running tests, I’m getting lots of warning messages like this one: /usr/lib/python3.5/json/decoder.py:355: ResourceWarning: unclosed <ssl.SSLSocket fd=21, family=AddressFamily.AF_INET, type=2049, proto=6, laddr=('127.0.0.1', 42742), raddr=('127.0.0.1', 47393)> — as we talked on zoom, I’m worried about resources not being fully freed after a test run (may create issues on Jenkins slaves), but if that’s not the case, I think it would still be good to avoid them for log readability purposes.
  • Also got some deprecation warnings: arvados/services/fuse/tests/test_exec.py:65: DeprecationWarning: Please use assertRegex instead. (check https://six.readthedocs.io/#unittest-assertions for py2/py3 compatibility)
  • services/fuse/arvados_fuse/fusedir.py - Lines 170 & 178: There’re some dict.itervalues() that were translated to dict.values() calls, that would use extra memory on python2, as it’s described on http://python-future.org/compatible_idioms.html#iterating-through-dict-keys-values-items, seems that the way to go is calling itervalues(dict)?
  • services/fuse/arvados_fuse/__init__.py:L276 — Using dict.items() is inefficient on Py2 as described on the previously mentioned guide list, maybe use viewitems(dict) or iteritems(dict)?
  • services/fuse/arvados_fuse/fusedir.py:L99 and services/fuse/arvados_fuse/__init__.py:L727 — Using list(dict.items()) is inefficient on Py2 as described on the previously mentioned guide list, maybe use listitems(dict)?

As for the manual tests, at first I initially got my arvbox shell instance to freeze when trying to write a file using py3 after trying it first with py2, but later on I did a proper test initially starting a shell and trying py2 and later closing the shell and re-starting it and doing the same with py3, and it worked without issues, so maybe I didn't unmounted the filesystem correctly at the first try.

Also, I think the documentation needs updating to state that it's compatible with Python3 https://doc.arvados.org/sdk/python/arvados-fuse.html

#15 Updated by Eric Biagiotti about 5 hours ago

Latest at 3ce47c5d012f9dbd6335ad0ae092eb9cc8a3022e

Unit tests: https://ci.curoverse.com/job/developer-run-tests/1404/ - Some workbench integration failures, but I ran them locally and they passed.
Conformance tests: https://ci.curoverse.com/view/CWL/job/arvados-cwl-conformance-tests/193/

Also retested manually in virtualenvs.

Lucas Di Pentima wrote:

  • When running tests, I’m getting lots of warning messages like this one: /usr/lib/python3.5/json/decoder.py:355: ResourceWarning: unclosed <ssl.SSLSocket fd=21, family=AddressFamily.AF_INET, type=2049, proto=6, laddr=('127.0.0.1', 42742), raddr=('127.0.0.1', 47393)> — as we talked on zoom, I’m worried about resources not being fully freed after a test run (may create issues on Jenkins slaves), but if that’s not the case, I think it would still be good to avoid them for log readability purposes.

I was able to reduce some of these with a1a1a8756149d5d05526503fd1e85115ea083fac, but the json and yaml ones have been difficult to pin down. I'm fairly certain these pertain to http2libs keep-alive feature https://github.com/httplib2/httplib2#keep-alive. Similar issues have been reported here https://github.com/kennethreitz/requests/issues/3912. The reason these only show up in testing is because they are normally suppressed by default in python, but unittest resets warning suppression.

They don't seem to be causing problems, but their impact on jenkins slaves is worth keeping in mind. It's also worth noting that these warnings show up in majority of our py3 test suites (sdk/python, sdk/cwl). For these reasons, I would like to leave the output as is and make a separate issue to investigate further. Let me know if you think this is satisfactory and I will make the issue.

Thanks for finding that! Fixed in latest.

  • services/fuse/arvados_fuse/fusedir.py - Lines 170 & 178: There’re some dict.itervalues() that were translated to dict.values() calls, that would use extra memory on python2, as it’s described on http://python-future.org/compatible_idioms.html#iterating-through-dict-keys-values-items, seems that the way to go is calling itervalues(dict)?
  • services/fuse/arvados_fuse/__init__.py:L276 — Using dict.items() is inefficient on Py2 as described on the previously mentioned guide list, maybe use viewitems(dict) or iteritems(dict)?
  • services/fuse/arvados_fuse/fusedir.py:L99 and services/fuse/arvados_fuse/__init__.py:L727 — Using list(dict.items()) is inefficient on Py2 as described on the previously mentioned guide list, maybe use listitems(dict)?

Fixed all three in latest.

Also, I think the documentation needs updating to state that it's compatible with Python3 https://doc.arvados.org/sdk/python/arvados-fuse.html

That page will be updated once we create the py3 arv-mount package. I'm tracking it here: https://dev.arvados.org/issues/15473

#16 Updated by Lucas Di Pentima about 4 hours ago

Eric Biagiotti wrote:

Latest at 3ce47c5d012f9dbd6335ad0ae092eb9cc8a3022e
[...]
They don't seem to be causing problems, but their impact on jenkins slaves is worth keeping in mind. It's also worth noting that these warnings show up in majority of our py3 test suites (sdk/python, sdk/cwl). For these reasons, I would like to leave the output as is and make a separate issue to investigate further. Let me know if you think this is satisfactory and I will make the issue.

Yes, thanks for doing the research! I wasn't aware of the warnings happening on other py3 test runs.

[...]

Also, I think the documentation needs updating to state that it's compatible with Python3 https://doc.arvados.org/sdk/python/arvados-fuse.html

That page will be updated once we create the py3 arv-mount package. I'm tracking it here: https://dev.arvados.org/issues/15473

Great, so this LGTM, thanks!

#18 Updated by Eric Biagiotti about 2 hours ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF