-
Notifications
You must be signed in to change notification settings - Fork 6
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
added first iteration of easy_publish #153
Changes from 10 commits
828b678
c2af2f3
9c6041f
4e0a17f
e16cc99
4812942
9f299b8
4e58649
760b703
8e1d3c6
03e948f
dc9fb36
0524c89
1fb6615
3e573d2
d4e6232
981efe7
5ba2463
026ef66
3521090
af04041
802cfc0
ba9e7e7
0ecdbbc
bf55004
c93efeb
e35b097
fcbd173
624a59b
83c126f
ac7f1d7
873f34c
5e8d8c6
6b1af9c
1a77bfd
3b08006
dbc7126
71cb851
5e5d34c
9310f71
ee975d7
f4ec3c2
b1eac69
e8576bd
08d255a
f7eb374
ba0a4fe
7cbfded
7213660
21a2ea0
88dab48
a22de1d
a9ef0c3
62b77be
666f5c8
4828b88
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,13 @@ | |
from mdf_toolbox.globus_search.search_helper import SEARCH_LIMIT | ||
from funcx.sdk.client import FuncXClient | ||
|
||
from dlhub_sdk.models.servables.keras import KerasModel | ||
from dlhub_sdk.models.servables.pytorch import TorchModel | ||
from dlhub_sdk.models.servables.python import PythonClassMethodModel | ||
from dlhub_sdk.models.servables.python import PythonStaticMethodModel | ||
from dlhub_sdk.models.servables.tensorflow import TensorFlowModel | ||
from dlhub_sdk.models.servables.sklearn import ScikitLearnModel | ||
|
||
from dlhub_sdk.config import DLHUB_SERVICE_ADDRESS, CLIENT_ID | ||
from dlhub_sdk.utils.futures import DLHubFuture | ||
from dlhub_sdk.utils.schemas import validate_against_dlhub_schema | ||
|
@@ -24,6 +31,10 @@ | |
logger = logging.getLogger(__name__) | ||
|
||
|
||
class HelpMessage(Exception): | ||
"""Raised from another error to provide the user an additional message""" | ||
|
||
|
||
class DLHubClient(BaseClient): | ||
"""Main class for interacting with the DLHub service | ||
|
||
|
@@ -336,6 +347,59 @@ def get_result(self, task_id, verbose=False): | |
result = result[0] | ||
return result | ||
|
||
def easy_publish(self, title: str, creators: Union[str, list[str]], short_name: str, servable_type: str, serv_options: dict[str, Any]): | ||
"""Simplified publishing method for servables | ||
|
||
Args: | ||
title (string): title for the servable | ||
creators (string | list): either the creator's name (FamilyName, GivenName) or a list of the creators' names | ||
short_name (string): shorthand name for the servable | ||
servable_type (string): the type of the servable, must be a member of ("static_method", | ||
"class_method", | ||
"keras", | ||
"pytorch", | ||
"tensorflow", | ||
"sklearn") | ||
serv_options (dict): the servable_type specific arguments that are necessary for publishing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add more context for the user on what the servable options are -- ideally, link to documentation |
||
Returns: | ||
(string): task id of this submission, can be used to check for success | ||
Raises: | ||
ValueError: If the given servable_type is not in the list of acceptable types | ||
Exception: If the serv_options are incomplete or the request to publish results in an error | ||
""" | ||
# conversion table for model string names to classes | ||
models = {"static_method": PythonStaticMethodModel, | ||
"class_method": PythonClassMethodModel, | ||
"keras": KerasModel, | ||
"pytorch": TorchModel, | ||
"tensorflow": TensorFlowModel, | ||
"sklearn": ScikitLearnModel} | ||
|
||
# raise an error if the provided servable_type is invalid | ||
model = models.get(servable_type) | ||
if model is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you wanted to shorten this a bit, you could do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would, but I also need to store the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤷♀️ I like "not in models" but not enough to change this here. |
||
raise ValueError(f"dl.easy_publish given invalid servable type: {servable_type}, please refer to the docstring") | ||
|
||
# attempt to construct the model and raise a helpful error if needed | ||
try: | ||
model_info = model.create_model(**serv_options) | ||
except Exception as e: | ||
help_err = HelpMessage(f"Help can be found here:\nhttps://dlhub-sdk.readthedocs.io/en/latest/source/dlhub_sdk.models.servables.html#" | ||
f"{model.__module__}.{model.__name__}.create_model") | ||
raise help_err from e | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to clarify, this help_err won't eat the original exception message, correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. specifying |
||
|
||
# set the required datacite fields | ||
model_info.set_title(title) | ||
creators = [creators] if isinstance(creators, str) else creators | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, so this is handling the case where the user inputs creators as a string instead of a list? please add a small inline comment saying as much (something like |
||
model_info.set_creators(creators, []) # we do not ask for affiliations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should allow the user to still set affiliations if they'd like to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be happy to talk about what parameters we want to provide for this function. When Logan and I discussed it, we opted to go for the absolute minimum and leave it to the publisher to edit later. All depends on where we want to draw the complexity line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do at least affiliations as a breakout argument. |
||
model_info.set_name(short_name) | ||
|
||
# perform the publish | ||
task_id = self.publish_servable(model_info) | ||
|
||
# return the id of the publish task | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great comments in this method Isaac, keep it up! 🎉 |
||
return task_id | ||
|
||
def publish_servable(self, model): | ||
"""Submit a servable to DLHub | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,6 +135,8 @@ def create_model(cls, path, method, function_kwargs=None) -> 'PythonClassMethodM | |
'class_name': class_name | ||
}) | ||
|
||
# func = getattr(obj, method) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this supposed to be removed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The branch that implements the automatic inspection writes over that line. So I suppose the answer to your question is yes, so I can add that the commit that addresses the other notes. |
||
|
||
return output | ||
|
||
def _get_handler(self): | ||
|
@@ -153,8 +155,8 @@ class PythonStaticMethodModel(BasePythonServableModel): | |
""" | ||
|
||
@classmethod | ||
def create_model(cls, module, method, autobatch=False, function_kwargs=None): | ||
"""Initialize the method | ||
def create_model(cls, module=None, method=None, autobatch=False, function_kwargs=None, *, f=None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to google this use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A little hard to explain in a comment here, but it's an issue of backwards compatibility for the |
||
"""Initialize the method based on the provided arguments | ||
ascourtas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Args: | ||
module (string): Name of the module holding the function | ||
|
@@ -163,7 +165,16 @@ def create_model(cls, module, method, autobatch=False, function_kwargs=None): | |
Calls :code:`map(f, list)` | ||
function_kwargs (dict): Names and values of any other argument of the function to set | ||
the values must be JSON serializable. | ||
f (object): A function pointer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this function pointer? what is it for? please add more context for the user There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I just copied that from the function that was already there. I can add more details |
||
Raises: | ||
TypeError: If there is no valid way to process the given arguments | ||
""" | ||
if f is not None: | ||
ascourtas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
module, method = f.__module__, f.__name__ | ||
elif module is None or method is None: | ||
raise TypeError("PythonStaticMethodModel.create_model was not provided valid arguments. Please provide either a funtion pointer" | ||
" or the module and name of the desired static function") | ||
|
||
output = super(PythonStaticMethodModel, cls).create_model(method, function_kwargs) | ||
|
||
output.servable.methods["run"].method_details.update({ | ||
|
@@ -175,13 +186,12 @@ def create_model(cls, module, method, autobatch=False, function_kwargs=None): | |
@classmethod | ||
def from_function_pointer(cls, f, autobatch=False, function_kwargs=None): | ||
"""Construct the module given a function pointer | ||
|
||
Args: | ||
f (object): A function pointer | ||
autobatch (bool): Whether to run function on an iterable of entries | ||
function_kwargs (dict): Any default options for this function | ||
""" | ||
return cls.create_model(f.__module__, f.__name__, autobatch, function_kwargs) | ||
return cls.create_model(f=f, autobatch=autobatch, function_kwargs=function_kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what to do about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that I understand.. |
||
|
||
def _get_handler(self): | ||
return 'python.PythonStaticMethodServable' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import mdf_toolbox | ||
from pytest import fixture, raises, mark | ||
from pytest_mock import mocker # your editor may fail to detect its usage | ||
|
||
from dlhub_sdk.models.servables.python import PythonStaticMethodModel | ||
from dlhub_sdk.utils.futures import DLHubFuture | ||
|
@@ -73,9 +74,9 @@ def test_run(dl): | |
assert res.result(timeout=60) == 'Hello world!' | ||
|
||
|
||
@mark.skipif(not is_gha, reason='Avoid running this test except on larger-scale tests of the system') | ||
@mark.skip | ||
def test_submit(dl): | ||
# @mark.skipif(not is_gha, reason='Avoid running this test except on larger-scale tests of the system') | ||
# @mark.skip | ||
def test_submit(dl, mocker): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice job with mocking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we go-ahead and remove the commented-out lines? |
||
# Make an example function | ||
model = PythonStaticMethodModel.create_model('numpy.linalg', 'norm') | ||
model.dlhub.test = True | ||
|
@@ -84,8 +85,24 @@ def test_submit(dl): | |
model.set_inputs('ndarray', 'Array to be normed', shape=[None]) | ||
model.set_outputs('number', 'Norm of the array') | ||
|
||
# make dummy reply for patch to return | ||
class DummyReply: | ||
def __init__(self) -> None: | ||
self.status_code = 200 | ||
|
||
def json(self) -> dict[str, str]: | ||
return {"task_id": "bf06d72e-0478-11ed-97f9-4b1381555b22"} # valid task id, status is known to be FAILED | ||
|
||
# patch requests.post | ||
mocker.patch("requests.post", return_value=DummyReply()) | ||
|
||
# Submit the model | ||
dl.publish_servable(model) | ||
task_id = dl.publish_servable(model) | ||
assert task_id == "bf06d72e-0478-11ed-97f9-4b1381555b22" | ||
|
||
# Submit the model using easy publish | ||
task_id = dl.easy_publish("Norm of a 1D Array", "Developer, Some", "1d_norm", "static_method", {"module": "numpy.linalg", "method": "norm"}) | ||
assert task_id == "bf06d72e-0478-11ed-97f9-4b1381555b22" | ||
|
||
|
||
def test_describe_model(dl): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ def _type_name_to_type(name: str) -> Union[type, Tuple[type]]: | |
try: | ||
return type_table.get(name) or __builtins__[name] # getattr(__builtins__, name) if __builtins__ is a module | ||
except KeyError: # AttributeError if __builtins__ is a module | ||
raise ValueError(f"found an unknown type name in servable metadata: {name}") | ||
raise ValueError(f"found an unknown type name in servable metadata: {name}") from None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if you raise from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same behavior will happen without the "from None," (i.e., what we had before), if I understand right There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the docs: "Exception chaining happens automatically when an exception is raised inside an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shows that I know. Let's keep yours |
||
|
||
|
||
def _generate_err(err_type: Exception, path: List[Tuple[str, Hashable]], expected: type = None, given: type = None, *, msg: str = None) -> Exception: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding an option for users to pass in a dict of datacite metadata if they'd like to (so have it be an optional dict parameter) would be ideal. How do you feel @WardLT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! We could still keep the "title" and "creators" as other arguments (that take precedent) so that people don't have to know datacite exists.