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

Service abstraction #171

Merged
merged 130 commits into from
Nov 21, 2024
Merged

Service abstraction #171

merged 130 commits into from
Nov 21, 2024

Conversation

wgifford
Copy link
Collaborator

Initial service abstraction

  • defines abstract classes for service handlers
  • defines huggingface service handler which supports huggingface specific conventions
  • defines model-specific service handles (derived from above handlers) that enables model-specific features

@wgifford wgifford force-pushed the service_abstraction branch from c36662a to 8907da8 Compare October 28, 2024 21:11
@wgifford wgifford marked this pull request as draft October 28, 2024 21:11
@ssiegel95
Copy link
Collaborator

I'll have a look at this tomorrow @wgifford, thanks.

@wgifford
Copy link
Collaborator Author

wgifford commented Oct 28, 2024

To do items:

  • Assess naming, adjust as needed.
  • Standardize what is needed in tsfm_config.json. Identify minimum requirements, requirements for HuggingFaceHandler, etc.
  • Do we still need a global confing.yaml? If not, remove. Keep for now.
  • Allow pulling the tsfm_config.json from HF (just like normal configs).
  • Resolve issue of model_id becoming the full path in the PredictionResult

from .hfutil import load_config, load_model, register_config
from .inference_payloads import ForecastingInferenceInput, PredictOutput
from .inference_payloads import ForecastingInferenceInput, ForecastingMetadataInput, PredictOutput
from .service_handler import ServiceHandler
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have expected to see an import of tsfm_service_handler here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We just need the base service handler -- it will be used to load the right handler class.

services/inference/tsfminference/inference.py Outdated Show resolved Hide resolved
services/inference/tsfminference/inference.py Show resolved Hide resolved
services/inference/tsfminference/inference.py Show resolved Hide resolved
services/inference/tsfminference/service_handler.py Outdated Show resolved Hide resolved
services/inference/tsfminference/service_handler.py Outdated Show resolved Hide resolved
LOGGER = logging.getLogger(__file__)


class ServiceHandler(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that there's a fair amount of implementation on this object, is it really an "abstract" base class, it seems to be just a run of the mill base class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

NM, I guess since you do have @abstractmethod decorators for some of the methods, you have no choice but to make it an ABC and I guess this makes it clearer that one is not to create an instance of a ServiceHandler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Though, I suppose this line: handler, e = ServiceHandler.load(model_path) is a bit odd, given ServiceHandler is abstract?

@wgifford wgifford marked this pull request as ready for review November 12, 2024 21:07
Copy link
Collaborator

@ssiegel95 ssiegel95 left a comment

Choose a reason for hiding this comment

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

This looks sensible.

@ssiegel95 ssiegel95 merged commit a5def44 into main Nov 21, 2024
12 checks passed
@ssiegel95 ssiegel95 deleted the service_abstraction branch November 21, 2024 20:51
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.

2 participants