Bug #2800

[Crunch] Remove global state in Python SDK

Added by Tom Clegg over 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
-
Target version:
Start date:
08/18/2014
Due date:
% Done:

100%

Estimated time:
(Total: 3.00 h)
Story points:
1.0

Description

It should be easy for a Python program to communicate with API and Keep storage servers at multiple sites in one process. For example:

a = arvados.api('v1', host='localhost', token='12345', insecure=True)
b = arvados.api('v1', host='qr1hi.arvadosapi.com', token='xyzzy')

x = a.collections().list().execute()

w1 = CollectionWriter(client=a)
w2 = CollectionWriter(client=b)
w1.write('foo')
w2.write('bar')

Subtasks

Task #3621: Review 2800-pgs branchResolvedTom Clegg

Task #3639: Review 2800-pysdk-no-global-keep-client-wipResolvedPeter Amstutz

Associated revisions

Revision 8917fa82
Added by Tom Clegg about 5 years ago

Merge branch '2800-pgs' refs #2800

Revision 05bea2c5
Added by Brett Smith about 5 years ago

Merge branch '2800-pysdk-no-global-keep-client-wip'

Closes #2800, #3639.

Revision 8598092d (diff)
Added by Brett Smith about 5 years ago

2800: Delay API client creation in Python CollectionReader.

This more closely matches prior behavior, and is necessary to make the
Keep tests pass. See included comments for detailed rationale.
Refs #2800.

History

#1 Updated by Tom Clegg over 5 years ago

  • Target version set to Arvados Future Sprints

#2 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#3 Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress
  • Assigned To set to Tom Clegg

#4 Updated by Tom Clegg about 5 years ago

Started 2800-python-global-state branch.

#5 Updated by Tom Clegg about 5 years ago

On 2800-pgs branch at c5f8a89

Interface
  • Accept host, token, and insecure arguments in the api() call. If provided, use them rather than looking up environment variables.
    • This change obsoletes some awkward maneuvers with environment variables (some of this removed from tests/test_api.py).
Caching
  • arvados.api() no longer returns a cached connection object with different credentials, endpoint, or "insecure" flag than the connection being requested. (Caching code used to assume only the requested version could change between calls to api().)
    • Example: this change makes it unnecessary for tests/test_api.py to clear the connection cache to prevent accidental future use of its fake credentials. Removed.
  • If the caller says cache=False, don't use an existing connection (as before), but don't put the newly created connection object in the cache either.
Token handling
  • The credentials supplied to (or looked up by) arvados.api() are used for the life of the connection it returns. Previously, CredentialsFromEnv looked up the auth token in the global os.environ during each API method call.
Mocking
  • test_api.py uses a mock for API requests but still needs a discovery document. Now it uses run_test_server.py instead of relying on qr1hi.arvadosapi.com. This makes the test suite slower but more meaningful.

#6 Updated by Tom Clegg about 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-08-27 Sprint

(The issue of using Keep services at multiple sites (global_client_object in keep.py) hasn't been addressed yet.)

#7 Updated by Brett Smith about 5 years ago

Just a few small comments on c5f8a89

  • We can skip the step of hashing strings for a cache key. We can just create a tuple of all the values we care about, and use that directly as the lookup key. What do you think of that approach?
  • Please keep imports sorted alphabetically.
  • Please retain the comment in test_api.py about needing a real discovery document, to explain why we're running a test server.

Thanks.

#8 Updated by Tom Clegg about 5 years ago

Brett Smith wrote:

  • We can skip the step of hashing strings for a cache key. We can just create a tuple of all the values we care about, and use that directly as the lookup key. What do you think of that approach?

Ah, yes, I was limiting myself to string keys for no reason. Now using tuple as dictionary key.

  • Please keep imports sorted alphabetically.

Fixed (by not importing hashlib)

  • Please retain the comment in test_api.py about needing a real discovery document, to explain why we're running a test server.

Indeed. Restored.

Thanks... now at 1e36870

#9 Updated by Brett Smith about 5 years ago

Tom Clegg wrote:

Brett Smith wrote:

  • Please keep imports sorted alphabetically.

Fixed (by not importing hashlib)

This is still an issue in test_api.py. Everything else looks good, so please go ahead and merge with that change. Thanks.

#10 Updated by Tom Clegg about 5 years ago

Fixed in 47dc22e. Thanks!

#11 Updated by Brett Smith about 5 years ago

  • Assigned To changed from Tom Clegg to Brett Smith

2800-pysdk-no-global-keep-client-wip is up for review to deprecate the global Keep client.

#12 Updated by Brett Smith about 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 33 to 100

Applied in changeset arvados|commit:05bea2c50474edeb9d0e3fb8daaf838b58ea9a54.

#13 Updated by Ward Vandewege about 5 years ago

  • Subject changed from Remove global state in Python SDK to [Crunch] Remove global state in Python SDK

Also available in: Atom PDF