Closed Bug 1245835 Opened 8 years ago Closed 8 years ago

python 3.5-compatible taskcluster client

Categories

(Taskcluster :: Services, defect)

Product:
Taskcluster
Component:
Services
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

Attachments

(5 files, 3 obsolete files)

output from broken_pgpy.py
1.49 KB, text/plain
Details
output from broken_pgpy.py (py2)
1.28 KB, text/plain
Details
broken_pgpy.py
4.03 KB, text/plain
Details
badstack.txt
1.79 KB, text/plain
Details
goodstack.txt
1.64 KB, text/plain
Details 
Catlee was working on one, with build-time generated code from the api definition.
(build-time allows for inspecting the code, while runtime does not)

He's lost his wip, and now I get to try.
Jonas and Catlee were gung ho for py35 asyncio, and various people saw no need for older python support.
Blocks: 1245837
Catlee regenerated a lot of his code generation wip here: https://github.com/catlee/tc
There are two kinds: events documenting amqp exchanges don't have baseUrl
10:51 <aki> jonasfj: i suppose the larger question is, does the client support using the amqp exchanges?
10:52 <•jonasfj> the python client doesn't...
10:52 <•jonasfj> the node client does
10:52 <aki> ok
10:52 <•jonasfj> the official pulse client is a bit old...
10:52 <•jonasfj> so writing a new one (an async) one could be really nice too...
10:53 <aki> :) i'll leave that as a separate task unless it blocks me
https://github.com/taskcluster/taskcluster-client.py/pull/37 for making taskcluster-client.py:mohawk pass flake8.
https://github.com/taskcluster/slugid.py/pull/4 for making slugid.py py35 compatible.
testAuthenticate test refactor work going on here: https://github.com/escapewindow/taskcluster-client.py/tree/testAuth

python3 work started here: https://github.com/escapewindow/taskcluster-client.py/tree/async
But:
* I'm going to try to refactor the tests first,
* I probably want to land the python3 fixes next,
* and then I'll work on the async stuff.

so the async branch will probably be renamed to py3 and rebased on top of the testAuth branch.
Green tests! using the testAuthenticate and testAuthenticateGet endpoints.

https://github.com/taskcluster/taskcluster-client.py/pull/41
The testAuthenticate PR (testAuth branch) is waiting for review.
In the meantime, I'm working on py3 unittest fixes on the py3 branch, which is based on the testAuth branch. https://github.com/escapewindow/taskcluster-client.py/tree/py3
I hit a py3 issue calling mohawk.  I could fix it on the tc.py side, but thought it might be good for mohawk to be able to handle this case.

https://github.com/kumar303/mohawk/pull/26
* Merged :dustin's https://github.com/escapewindow/taskcluster-client.py/pull/1 into the testAuth branch after running some local tests.
* Rebased the py3 branch on top of the updated testAuth branch -- some conflicts, but all fixable.
* With :dustin's fixes, we no longer need https://github.com/kumar303/mohawk/pull/26 for green tests.
* Currently at green py27, and py35 is 6 failed/2 errors/39 passed.  (not sure why the test count is so low given 116 py27 tests, maybe because non-valid python3 syntax)
(In reply to Aki Sasaki [:aki] from comment #14)
> * With :dustin's fixes, we no longer need
> https://github.com/kumar303/mohawk/pull/26 for green tests.

That's actually not true.
Amended:

With :dustin's fixes, new errors hid the need for https://github.com/kumar303/mohawk/pull/26 (which is now merged).

Now that I've cleared up some of the new errors, I hit the same mohawk issue.  I updated to the patched version and am now at 1 failed/8 errors/107 passed for py35.
The testAuth branch is now merged! (thanks :jhford and :dustin)

* The mohawk issue was a side effect of broken py35 bewits in my taskcluster-client.py branch.
** this is now fixed in my branch, along with a ton of debug prints etc
* currently at 1 failed / 115 passed; dustin helped me debug the signed url bustage.
** The failed test is the str -> binary -> base64 -> pgp encoded -> decrypted -> base64 -> broken binary string.  One of these transformations is breaking things.  I'm going to continue with my approach of debug prints to track this down.  Hopefully I get green py35 tests tomorrow, so I can clean up the code + commits to be ready for review by my EOD.

After py3 tests are ready, I'll work on codegen and generated code tests, and then async.
I think I've narrowed it down.
It's getting munged in pgpy.PGPMessage before it's even encrypted.

    message = "Hello 
Ugh, unicode broke bugzilla.  Trying again.


I think I've narrowed it down.
It's getting munged in pgpy.PGPMessage before it's even encrypted.

    message = "Hello \U0001F4A9!"
    msg = pgpy.PGPMessage.new(message)
    print(msg.message)

gives

    bytearray(b'Hello \x01\xf4\xa9!')

And that's broken:

>>> bytearray(b'Hello \x01\xf4\xa9!').decode('utf-8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xf4 in position 7: invalid continuation byte

And the above all happens even before this line:

    encrypted = key.encrypt(msg)


This is only broken on py35.
The question is, is there a way to get this working without patching pgpy?  I'll be trying various ways tomorrow (Wed).
The bustage happens in PGPObject.text_to_bytes:

https://github.com/SecurityInnovation/PGPy/blob/master/pgpy/types.py#L227

This method gives me an easy workaround: send it a bytearray, and the bytearray passes through without munging.

When I

    msg = pgpy.PGPMessage.new(bytearray(message, encoding="utf-8"))

and then on decryption,

    message = decrypted.message
    if isinstance(message, bytearray):
        message = message.decode('utf-8')

the tests go green.  116 tests passed!


I gotta clean up my branch to be reviewable.

I want to also write a couple tests for PGPy and submit a PR and issue.  If I leave it open with STR in a test case, great.  If I can patch it so it works for real in PGPy and get that merged, even better.
Attached file broken_pgpy.py (test script) (obsolete) — 

    
    

    
  

      
      
      
      

        Attached file
          
        broken_pgpy.py (obsolete)
        — 
      

    


  
This is a little cleaner than the original.  Still not proud of the globals or other assumptions, but it shows what's working and what's broken.

        Attachment #8723186 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  

      
      
      
      

        Attached file
          
        broken_pgpy.py (obsolete)
        — 
      

    


  
This one works in py2 but doesn't in py3.  Probably more useful to show what's different.

        Attachment #8723226 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        broken_pgpy.py
        — 
      

    


  

        Attachment #8723272 -
        Attachment is obsolete: true

    
    

    
  
There's a 'develop' branch in pgpy.  I checked it out to see if this issue is already fixed.  Still broken.
https://github.com/SecurityInnovation/PGPy/compare/develop

    
    

    
  
https://github.com/taskcluster/taskcluster-client.py/pull/44 for merging py3 branch into taskcluster-client.py master.

    
    

    
  
* PGPy issue is fixed: https://github.com/SecurityInnovation/PGPy/issues/154
** PGPy 0.4.0 will have the fix, and it's scheduled for Soon https://github.com/SecurityInnovation/PGPy/issues/154#issuecomment-189557355

* py3 branch is merged in as of Monday morning PT
* I rebased my codegen branch onto the new master

* codegen is now working for synchronous code, all tests green!  I have some cleanup and codegen specific tests to run.  https://github.com/escapewindow/taskcluster-client.py/tree/codegen
** I have a bad stackdump (pre-codegen) and a good stackdump (post-codegen) to attach for comparison.

I'll clean up the branch for review tomorrow, then start a new branch for the async work.

    
    

    
  

      
      
      
      

        Attached file
          
        badstack.txt
        — 
      

    


  
This is confusing because apiCall is just this function in createApiClient:
https://github.com/taskcluster/taskcluster-client.py/blob/fe3b1f8c9520184aee560f34d370632352da9962/taskcluster/client.py#L480

    
    

    
  

      
      
      
      

        Attached file
          
        goodstack.txt
        — 
      

    


  
This actually points at File "/Users/asasaki/src/taskcluster-client/taskcluster-client.py/taskcluster/generated/Auth.py", line 443, in testAuthenticate

That's my understanding of the stackdump problem and fix.

    
    

    
  
Oh, I also have to verify the topic-exchange api support... might not be fully baked.

    
    

    
  
:garbas, do the above attachments fix the bad/confusing stack traces you were seeing before?
original: https://bugzilla.mozilla.org/attachment.cgi?id=8725122
new: https://bugzilla.mozilla.org/attachment.cgi?id=8725124
Flags: needinfo?(rok)

    
    

    
  
:aki not really. my problem is that methods are generated from json at import/run time. which makes introspection hard and errors you get when providing too few/many/wrong arguments, just WTF? basically the approach taken by the code is really how you would provide a client in python. but i guess this we can solve at later time (not being scope of this ticket).
Flags: needinfo?(rok)

    
    

    
  
(In reply to Rok Garbas [:garbas] from comment #34)
> :aki not really. my problem is that methods are generated from json at
> import/run time.

The 2nd attachment is from methods generated form json at buildtime.  Did that not fix it?
Flags: needinfo?(rok)

    
    

    
  
Essentially, if that doesn't fix the problem, the argument to generate the methods at buildtime is much weaker.

    
    

    
  
10:44 <catlee> aki, garbas: having code generated also means the functions (should) have arguments that make sense, and can be autocompleted in your editor?
10:44 <aki> that's true

    
    

    
  
Updated the generated code PR with review comment fixes: https://github.com/taskcluster/taskcluster-client.py/pull/45

I gave the async split a decent amount of thought.  There are enough time.sleep()s and requests hardcodes in utils and client that I'm thinking about creating a taskcluster.sync.session and taskcluster.sync.client that contain the synchronous, requests code, and taskcluster.async versions for the asynchronous, aiohttp code.

As long as py2 doesn't import any taskcluster.async code, and as long as py2 flake8 has an --exclude=async, the existence of py3 code in the tree seems to be ok.

    
    

    
  
:aki: great work! i guess we could also generate tests, right?
Flags: needinfo?(rok)

    
    

    
  
:garbas thank you.  Yes, generated tests are also in https://github.com/taskcluster/taskcluster-client.py/pull/45 .

    
    

    
  
I also added fixes for taskcluster-client.py for existing bugs uncovered in the review (no querystring support, wrong retry backoff math): https://github.com/taskcluster/taskcluster-client.py/pull/45

Waiting on review/merge there.

"Rebased" the async branch -- because I reordered and squashed the commits on the codegen branch for cleaner review, I had to rebranch and cherry-pick the couple small commits.  I'll be working on that branch until I hear back in the PR.

    
    

    
  
* https://github.com/taskcluster/taskcluster-client.py/pull/45 is now closed, due to length.  However, it's close to merge-able.  I'm going to tweak the integration tests to cover both runtime- and buildtime-generated classes, then open a new PR.

    
    

    
  
* the generated code pr is merged as of last week.
* I'm working on the generated async code in the async branch https://github.com/escapewindow/taskcluster-client.py/tree/async
* I got bogged down in a lot of the code changes, so I decided I'd be best served writing a small piece of code that I could demonstrate was async: https://gist.github.com/escapewindow/e9c6ded64196f40ffa28

Now that I have that, I can try to get that into the AsyncClient object.

    
    

    
  
https://gist.github.com/escapewindow/e9c6ded64196f40ffa28#file-aki-py proves my new generated async Auth is async.  Working on generated async tests.

    
    

    
  
(In reply to Aki Sasaki [:aki] from comment #46)
> https://gist.github.com/escapewindow/e9c6ded64196f40ffa28#file-aki-py proves
> my new generated async Auth is async.  Working on generated async tests.

Er, no it doesn't. tried to figure out how to do it with a simple script, but testAuthenticate returns too quickly to test async.  I think my tests-in-progress may be the fastest way to verify the async-ness of the new classes.

    
    

    
  
The mocked async tests are green!  That means the methods are called appropriately and are async... I have tests that verify that a failed first call is followed by an unblocked second call, then followed by a retry of the first call.

However, the async integration tests are hanging.  This probably means my async _makeHttpRequest() is broken somewhere.  I'm narrowing down by a lot of debug prints.

    
    

    
  
Woo, integration tests are (all but one) green!

* I now have more simple scripts that use the various functions.  These helped me debug/get each function working without as much noise: https://gist.github.com/escapewindow/e9c6ded64196f40ffa28
** still need to fix no_creds_needed, which uses httmock, which is requests specific.  I'm going to check out https://github.com/BenjamenMeyer/aiohttp-mock
* coverage < 4.1 barfs on async! https://bitbucket.org/ned/coveragepy/issues/434/indexerror-in-python-35
* still want to increase the async code coverage
* still want to clean up the code for reviewability

but I think the hardest parts are done.

    
    

    
  
* I fixed the no_creds_needed async integration test, and added a multi-async-integration test.  (These two differ from the other mocked calls because I patch aiohttp.ClientSession._request deep in the call stack, rather than mocking AsyncClient._makeHttpRequest in the non-integration async tests)

* 100% async code coverage

* found two largely dup code blocks between SyncClient and AsyncClient.  One of them was easily moved into a BaseClient helper method.

* rebased the branch

* created the PR: https://github.com/taskcluster/taskcluster-client.py/pull/49

:jhford let me know this review pass may take longer than the previous PRs.  I'll be working on bug 1245837, based on my async tc.py code, in the meantime.

    
    

    
  
async landed in taskcluster-client.py, and :jhford released version 0.3.0 with async on pypi.

I think we're done here.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Client Libraries → Services

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: