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

148 add capability to run model using only model doi #180

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

isaac-darling
Copy link
Contributor

Just an addition to the current run function to support DOIs as input. This seemed like the easiest solution to maintain, but of course could be switched to a stand-alone function.

@isaac-darling isaac-darling added the enhancement New feature or request label Nov 9, 2022
@isaac-darling isaac-darling self-assigned this Nov 9, 2022
@isaac-darling isaac-darling linked an issue Nov 9, 2022 that may be closed by this pull request
@isaac-darling
Copy link
Contributor Author

Errors appear to be resultant of a timeout, which I am not sure how to address at this moment.

@ascourtas
Copy link
Contributor

@isaac-darling Ben recently made some changes (to either dev or main) that fixed these failing tests, could you pull those in so we can review your work?

@@ -258,7 +258,7 @@ def run(self, name: str, inputs: Any, parameters: Optional[Dict[str, Any]] = Non
"""Invoke a DLHub servable

Args:
name: DLHub name of the servable of the form <user>/<servable_name>
identifier: DOI of desired servable or DLHub name of the form <user>/<servable_name>
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of replacing name, can you keep both? so the user has the option of supplying either the name or the identifier DOI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it is, identifier can be a name or a DOI. only the name of the variable was changed. if that is a problem in and of itself, though, then of course I can add another input (although that would be a little clunky IMO because the user would have to use it by name)


if name not in self.fx_cache:
# if identifier is a DOI
if identifier.startswith("10.") or identifier.startswith("https://doi.org/"):
Copy link
Contributor

Choose a reason for hiding this comment

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

what if it's not a properly formatted DOI? Please throw an error to let the user know that their input was incorrect. Otherwise, this will probably hang or yield an uninformative error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an error will be thrown if the DOI is not found (correct or not), which will at least tell them where to look. would you prefer a specific check for valid formatting?

@coveralls

This comment was marked as duplicate.

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3472256575

  • 7 of 12 (58.33%) changed or added relevant lines in 1 file are covered.
  • 7 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.4%) to 79.676%

Changes Missing Coverage Covered Lines Changed/Added Lines %
dlhub_sdk/client.py 7 12 58.33%
Files with Coverage Reduction New Missed Lines %
dlhub_sdk/models/servables/tensorflow.py 1 0%
dlhub_sdk/models/servables/tests/test_tensorflow.py 3 21.25%
dlhub_sdk/utils/futures.py 3 74.42%
Totals Coverage Status
Change from base Build 3452624594: -0.4%
Covered Lines: 1623
Relevant Lines: 2037

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add capability to run model using only model DOI
3 participants