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

[SYNPY-1322] Object Orientated Programming Interfaces #1013

Merged
merged 34 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
625b0ba
OTEL Additions to what is/isn't a span
BryanFauble Nov 10, 2023
6105a71
>= dependencies
BryanFauble Nov 10, 2023
cebe138
Add in document and script around benchmarking
BryanFauble Nov 14, 2023
89cd7c7
Code review feedback
BryanFauble Nov 16, 2023
40250fb
Link to tutorial section
BryanFauble Nov 16, 2023
dd0eeb0
Adjust OTEL dependencies
BryanFauble Nov 16, 2023
03cf453
Mark test as flaky
BryanFauble Nov 16, 2023
dcc29f0
Publish branch to remote
BryanFauble Nov 16, 2023
77a8d6b
Import File earlier
BryanFauble Nov 16, 2023
b27cf12
Merge branch 'develop' into SYNPY-1322-OOP-POC
BryanFauble Nov 16, 2023
244c69f
Support for annotations,cache syn client instance
BryanFauble Nov 20, 2023
55ee852
Remove httpx
BryanFauble Nov 20, 2023
65a1c70
Merge branch 'develop' into SYNPY-1322-OOP-POC
BryanFauble Nov 28, 2023
57da4e1
Adding support for Table
BryanFauble Nov 30, 2023
5bdf4c4
Adding delete row note
BryanFauble Nov 30, 2023
1a11b17
Adding delete table
BryanFauble Nov 30, 2023
f988b5c
More example scripts
BryanFauble Nov 30, 2023
5d5f9f5
Remove comment
BryanFauble Nov 30, 2023
6b5b9af
Prevent creating instance for calling class method
BryanFauble Nov 30, 2023
86a474e
Correcting OTEL context propogation
BryanFauble Dec 1, 2023
dfbddcc
Add note about assuming annotation type
BryanFauble Dec 1, 2023
6b74371
Merge branch 'develop' into SYNPY-1322-OOP-POC
BryanFauble Dec 7, 2023
84a5369
Adding more verbose examples for current project interface
BryanFauble Dec 8, 2023
d370b46
simplify the annotation interface (#1022)
BryanFauble Dec 12, 2023
3aec4ff
[SYNPY-1345] Migrate to mkdocstrings (#1023)
BryanFauble Dec 12, 2023
93bacca
Making sticky nav tabs per team meeting
BryanFauble Dec 12, 2023
bbe905f
Merge branch 'develop' into SYNPY-1322-OOP-POC
BryanFauble Jan 16, 2024
410de22
Remove content not needed
BryanFauble Jan 16, 2024
be9f193
Remove content that is not needed
BryanFauble Jan 16, 2024
cc049b0
Merge develop and consistency changes
BryanFauble Jan 16, 2024
53e4c22
Merge branch 'develop' into SYNPY-1322-OOP-POC
BryanFauble Jan 17, 2024
8fef247
Correct for some changes
BryanFauble Jan 17, 2024
a19d77a
Clean up doc structure
BryanFauble Jan 22, 2024
377df74
Code review updates
BryanFauble Jan 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions synapseclient/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
SubmissionViewSchema,
MaterializedViewSchema,
Dataset,
delete_rows,
)
from .team import Team, UserProfile, UserGroupHeader, TeamMember
from .wiki import Wiki
Expand Down Expand Up @@ -69,6 +70,7 @@
# functions
"login",
"build_table",
"delete_rows",
"as_table_columns",
"check_for_updates",
"release_notes",
Expand Down
7 changes: 7 additions & 0 deletions synapseclient/api/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# These are all of the models that are used by the Synapse client.
from .annotations import set_annotations


__all__ = [
"set_annotations",
]
37 changes: 37 additions & 0 deletions synapseclient/api/annotations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
"""
The purpose of this module is to provide any functions that are needed to interact with
annotations that are not cleanly provided by the synapseclient library.
"""
import json

from dataclasses import asdict

from typing import TYPE_CHECKING, Optional
from synapseclient import Synapse

if TYPE_CHECKING:
from synapseclient.models import Annotations


def set_annotations(
annotations: "Annotations", synapse_client: Optional[Synapse] = None
):
"""Call to synapse and set the annotations for the given input.

:param annotations: The annotations to set. This is expected to have the id, etag, and annotations filled in.
:return: _description_
"""
annotations_dict = asdict(annotations)

# TODO: Is there a more elegant way to handle this - This is essentially being used
Copy link
Contributor

@BWMac BWMac Nov 21, 2023

Choose a reason for hiding this comment

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

There is another dataclass library that adds some more functionality. Its asdict function has an exclude argument that you can pass to leave out class attributes you don't want in your dictionary:

from dataclasses import dataclass
from dataclass_wizard.dumpers import asdict

@dataclass
class Foo:
    bar: str
    baz: int

foo = Foo('bar', 42)
print(asdict(foo, exclude=['baz']))

> {'bar': 'bar'}

You'd still be hard-coding the excluded values though. Not sure of any other ways aside from implementing an asdict method specific to the class that excludes attributes not used in the API.

Edit: if you used a base class you could potentially implement an asdict function that could be reused across all extending classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - but i'm starting to think in the other direction now. What I mean by this is specify only those things I want to include and have a dataclass -> json/dict mapping process to format the input for the REST API as the API is expecting.

# TODO: to remove any fields that are not expected by the REST API.
filtered_dict = {
k: v for k, v in annotations_dict.items() if v is not None and k != "is_loaded"
}

# TODO: This `restPUT` returns back a dict (or string) - Could we use:
# TODO: https://github.com/konradhalas/dacite to convert the dict to an object?
Copy link
Contributor

@BWMac BWMac Nov 21, 2023

Choose a reason for hiding this comment

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

I do like the look of dacite, but is that more efficient than just wrapping the response in the class object like we have done elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure yet - I'll have to give it some more thought.

One of the things we will need to do it at least have a thin translation layer between the REST api and the dataclasses because the names we are giving them in the python client are snake_case, vs the REST api is all in camelCase.

return Synapse.get_client(synapse_client=synapse_client).restPUT(
f"/entity/{annotations.id}/annotations2",
body=json.dumps(filtered_dict),
)
33 changes: 33 additions & 0 deletions synapseclient/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,8 @@ class Synapse(object):

"""

_synapse_client = None

# TODO: add additional boolean for write to disk?
@tracer.start_as_current_span("Synapse::__init__")
def __init__(
Expand Down Expand Up @@ -300,6 +302,30 @@ def _init_logger(self):
self.logger = logging.getLogger(logger_name)
logging.getLogger("py.warnings").handlers = self.logger.handlers

@classmethod
def get_client(cls, synapse_client: None) -> "Synapse":
"""
Convience function to get an instance of 'Synapse'. The latest instance created
by 'login()' or set via `set_client` will be returned.

When 'logout()' is called it will delete the instance.

:param syn: An instance of 'Synapse' or None. This is used to simplify logical checks
in cases where synapse is passed into them.
"""
if synapse_client:
return synapse_client

if not cls._synapse_client:
raise SynapseError(
"No instance has been created - Please use login() first"
)
return cls._synapse_client

@classmethod
def set_client(cls, synapse_client) -> None:
cls._synapse_client = synapse_client

@property
def max_threads(self) -> int:
return self._max_threads
Expand Down Expand Up @@ -401,6 +427,7 @@ def login(
silent: bool = False,
forced: bool = False,
authToken: str = None,
cache_client: bool = True,
):
"""
Valid combinations of login() arguments:
Expand Down Expand Up @@ -428,6 +455,9 @@ def login(
credential storage.
:param authToken: A bearer authorization token, e.g. a personal access token, can be used in lieu of a
password or apiKey
:param cache_client: Whether to cache the Synapse client object in the Synapse module. Defaults to True.
When set to True anywhere a `Synapse` object is optional you do not need to pass an
instance of `Synapse` to that function, method, or class.

**GNOME Keyring** (recommended) or **KWallet** is recommended to be installed for credential storage on
**Linux** systems.
Expand Down Expand Up @@ -515,6 +545,8 @@ def login(
else self.credentials.username
)
)
if cache_client:
Synapse.set_client(self)

def _get_config_section_dict(self, section_name: str) -> dict:
config = self.getConfigFile(self.configPath)
Expand Down Expand Up @@ -614,6 +646,7 @@ def logout(self, forgetMe: bool = False):
self.credentials.delete_from_keyring()

self.credentials = None
Synapse.set_client(None)

def invalidateAPIKey(self):
"""Invalidates authentication across all clients."""
Expand Down
3 changes: 1 addition & 2 deletions synapseclient/core/retry.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import random
import sys
import logging
from opentelemetry import trace

from synapseclient.core.logging_setup import DEBUG_LOGGER_NAME, DEFAULT_LOGGER_NAME
from synapseclient.core.utils import is_json
from synapseclient.core.dozer import doze
from opentelemetry import trace

DEFAULT_RETRIES = 3
DEFAULT_WAIT = 1
Expand All @@ -25,7 +25,6 @@
"couldn't connect to host",
"slowdown",
"try again",
"connection reset by peer",
]

# Exceptions that may be retryable. These are socket level exceptions
Expand Down
6 changes: 5 additions & 1 deletion synapseclient/core/upload/multipart_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,11 @@ def multipart_upload_file(
"""

trace.get_current_span().set_attributes(
{"synapse.storage_location_id": storage_location_id}
{
"synapse.storage_location_id": storage_location_id
if storage_location_id is not None
else ""
}
)

if not os.path.exists(file_path):
Expand Down
34 changes: 34 additions & 0 deletions synapseclient/models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# These are all of the models that are used by the Synapse client.
from synapseclient.models.annotations import (
Annotations,
AnnotationsValue,
AnnotationsValueType,
)
from synapseclient.models.file import File
from synapseclient.models.folder import Folder
from synapseclient.models.project import Project
from synapseclient.models.table import (
Table,
Column,
ColumnType,
FacetType,
CsvResultFormat,
RowsetResultFormat,
Row,
)

__all__ = [
"File",
"Folder",
"Project",
"Annotations",
"AnnotationsValue",
"AnnotationsValueType",
"Table",
"Column",
"ColumnType",
"FacetType",
"CsvResultFormat",
"RowsetResultFormat",
"Row",
]
1 change: 1 addition & 0 deletions synapseclient/models/activity.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# TODO
112 changes: 112 additions & 0 deletions synapseclient/models/annotations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import asyncio

from enum import Enum
from dataclasses import dataclass
from typing import Dict, List, Optional, Union
from synapseclient.api import set_annotations
from opentelemetry import trace

from synapseclient import Synapse


tracer = trace.get_tracer("synapseclient")


class AnnotationsValueType(str, Enum):
"""The acceptable types that an annotation value can be."""

STRING = "STRING"
DOUBLE = "DOUBLE"
LONG = "LONG"
TIMESTAMP_MS = "TIMESTAMP_MS"
BOOLEAN = "BOOLEAN"


@dataclass()
class AnnotationsValue:
"""A specific type of annotation and the values that are of that type."""

type: AnnotationsValueType
# TODO: What are all the python types we are going to accept here
value: List[Union[str, bool]]


@dataclass()
class Annotations:
BryanFauble marked this conversation as resolved.
Show resolved Hide resolved
"""Annotations that can be applied to a number of Synapse resources to provide additional information."""

annotations: Dict[str, AnnotationsValue]
""" Additional metadata associated with the object. The key is the name of your
desired annotations. The value is an object containing a list of string values
(use empty list to represent no values for key) and the value type associated with
all values in the list
"""

id: Optional[str] = None
"""ID of the object to which this annotation belongs. Not required if being used as
a member variable on another class."""

etag: Optional[str] = None
""" Etag of the object to which this annotation belongs. To update an AnnotationV2,
this field must match the current etag on the object. Not required if being used as
a member variable on another class."""

is_loaded: bool = False

async def store(
self,
synapse_client: Optional[Synapse] = None,
):
"""Storing annotations to synapse."""
# TODO: Validation that id and etag are present

print(f"Storing annotations for id: {self.id}, etag: {self.etag}")
with tracer.start_as_current_span(f"Annotation_store: {self.id}"):
loop = asyncio.get_event_loop()
result = await loop.run_in_executor(
None,
lambda: set_annotations(
annotations=self, synapse_client=synapse_client
),
)
print(f"annotations store for {self.id} complete")
self.annotations = Annotations.convert_from_api_parameters(result)
# TODO: From the returned call do we need to update anything in the root object?
return self

async def get(self):
"""Get the annotations from synapse."""
print(f"Getting annotations for id: {self.id}, etag: {self.etag}")
await asyncio.sleep(1)
self.is_loaded = True

@classmethod
def convert_from_api_parameters(
self, synapse_annotations: dict
) -> Dict[str, AnnotationsValue]:
"""Convert the annotations from the synapse API to the model."""
# TODO: This is not great logic and needs to be revisted. Ideally the annotations
# TODO: returned as the same during a `.get` and `.store` call. Currently they are not
# TODO: This also prevents us from using the annotations returned from a `.get` call to store them again.
# TODO: Also there is difference in timestamp being transferred - The API is expecting milliseconds
# TODO: But in most cases the python client is returning datetime.
if synapse_annotations is None:
return None
BryanFauble marked this conversation as resolved.
Show resolved Hide resolved
annotations = {}
dict_to_convert = (
synapse_annotations["annotations"]
if "annotations" in synapse_annotations
else synapse_annotations
)
for key in dict_to_convert:
# TODO: How can we determine which type is being used when it is not provided in the response from the python client.
value = (
dict_to_convert[key]["value"]
if "value" in dict_to_convert[key]
else dict_to_convert[key]
)
annotations[key] = AnnotationsValue(
type=None,
value=value,
)
return annotations
1 change: 1 addition & 0 deletions synapseclient/models/evaluation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# TODO
Loading
Loading