Skip to content

Conversation

@badGarnet
Copy link
Collaborator

This PR allows us to register new models and use them in the system:

  • refactor of get_model so it relies on information stored in model_class_map and model_config_map to initialize a new model with a given model name (which is the key to both mappings)
  • a new function unstructured_inference.models.base.register_new_model now allows adding new model definition to the class mapping and config mapping
  • after calling register new model one can now call get_model with the new model name and get the new model

testing

New unit tests should pass

@badGarnet badGarnet marked this pull request as ready for review April 3, 2024 18:49
Copy link
Contributor

@scanny scanny left a comment

Choose a reason for hiding this comment

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

LGTM. One suggestion on typing conventions.

Comment on lines 26 to 28
Tuple[
Dict[str, Type[UnstructuredModel]],
Dict[str, Union[dict, LazyDict]],
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add:

from __future__ import annotations

at the top of the file you can use modern notation for union and tuple types like:

tuple[
    dict[str, type[UnstructuredModel]],
    dict[str, dict | LazyDict],
]

which does not require the imports from typing (for those types) and is consistent with how Python and IDEs display types in messages etc.

Btw, a bare dict is not a type. Here maybe it's dict[str, Any].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah; forgot about future... I was back and forth with lint there 😅

@badGarnet badGarnet enabled auto-merge (squash) April 4, 2024 20:34
@badGarnet badGarnet merged commit 147c5b1 into main Apr 4, 2024
@badGarnet badGarnet deleted the refactor/get-model-allow-registering-of-new-models-with-function-calls branch April 4, 2024 20:56
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.

3 participants