Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create versioned Compute clients #1096

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

rjmello
Copy link
Member

@rjmello rjmello commented Oct 31, 2024

Created ComputeClientV2 and ComputeClientV3 classes to support Globus Compute API versions 2 and 3, respectively. The canonical ComputeClient is now a subclass of ComputeClientV2, preserving backward compatibility.


📚 Documentation preview 📚: https://globus-sdk-python--1096.org.readthedocs.build/en/1096/

src/globus_sdk/services/compute/client.py Outdated Show resolved Hide resolved
@rjmello rjmello force-pushed the versioned-compute-clients branch from 0ad25f6 to fa05028 Compare October 31, 2024 20:57
@sirosen sirosen force-pushed the versioned-compute-clients branch from e0bf9ac to 88c4abf Compare October 31, 2024 22:24
Copy link
Contributor

@khk-globus khk-globus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending the outcome of the resolution order discussion, the rest of the PR looks on-target to me.

docs/services/compute.rst Show resolved Hide resolved
src/globus_sdk/_testing/registry.py Show resolved Hide resolved
@rjmello rjmello force-pushed the versioned-compute-clients branch from 88c4abf to b57bcbb Compare November 4, 2024 23:42
@sirosen sirosen dismissed their stale review November 5, 2024 02:45

As I have become a coauthor, recusing myself from review

rjmello and others added 3 commits November 5, 2024 11:37
Created `ComputeClientV2` and `ComputeClientV3` classes to support
Globus Compute API versions 2 and 3, respectively. The canonical
`ComputeClient` is now a subclass of `ComputeClientV2`, preserving
backward compatibility.
When mapping client objects (by name) to directories in the _testing
data modules, the current mapping inspects `service_name` and the
method name. As a result, it's not possible to have multiple versioned
clients with the same service name mapped to different fixture
directories.
To resolve, we need *some* solution which maps versioned clients to
versioned fixtures.

This change applies one particular solution, which will work in the
near-term for ComputeClientV2 and ComputeClientV3, but may be refined
further in the future (so long as the mapping of the V2 client to
known fixture data is preserved).

Rather than ignoring the possibility of structured information in the
name of a client, if it ends with `V<N>` for some integer `N`, we can
map this to a fixture dir named, similarly, `v<N>` (lowercased). This
allows the V2 and V3 clients to get their own fixture directories.

One downside of this approach is that it maps `ComputeClient` (which
inherits from the V2 client) differently from `ComputeClientV2`, and
there is no step here taken to rectify this. Effectively, this means
that `ComputeClient` will get no fixture data, but `ComputeClientV2`
will, even for identical usages.

Alternative approaches abound -- e.g., we could attach a class-level
attribute which declares a "test_fixture_subdir" or "api_version", and
use that attribute when present. The current solution was chosen as
- generic, in that it does not encode Compute-specific details into a
  helper buried in `_testing`
- easy to implement and reason about (it's just a quick suffix match
  and string conversion)
- easy to change or replace in the future, because it makes no
  changes to the classes which are fed into this mapping
@rjmello rjmello force-pushed the versioned-compute-clients branch from b57bcbb to 7d63262 Compare November 5, 2024 16:38
@rjmello rjmello requested a review from derek-globus November 5, 2024 19:22
Copy link
Contributor

@derek-globus derek-globus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the slow review; we've been juggling a few different things within Automate.

@kurtmckee kurtmckee merged commit 2ba95ce into globus:main Nov 6, 2024
16 checks passed
@rjmello rjmello deleted the versioned-compute-clients branch November 6, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants