Danyil/feature/use marqo commons #607
Conversation
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
| @@ -60,6 +59,7 @@ class CLIP_ONNX(object): | |||
| def __init__(self, model_name: str ="onnx32/openai/ViT-L/14", device: str = None, embedding_dim: int = None, | |||
| truncate: bool = True, | |||
| load=True, **kwargs): | |||
| from marqo.s2_inference.s2_inference import get_model_properties_from_registry | |||
There was a problem hiding this comment.
Why is this not declared at the top of the file?
There was a problem hiding this comment.
due to a circular import issue, we'd have to resolve it. maybe onnx clip object can work without that function but it needs deeper investigation
There was a problem hiding this comment.
@pandu-k Do you have any input on resolving this circular import? Declaring it in this init function works but it feels a bit hacky. The issue is that onnx_clip_utils here calls s2_inference, which imports from model_loaders, which imports from onnx_clip_utils.
There was a problem hiding this comment.
This strange situation is an indication that our package structure isn't ideal. Good pick up @vicilliar!
I think the best way is for the model_registry is purely for static data (a.k.a the model properties).
We can then create a new file that has _get_model_load_mappings() and load_model_properties(). Would that solve this?
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
vicilliar
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
vicilliar
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
danyilq
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
vicilliar
had a problem deploying
to
marqo-test-suite
GitHub Actions
Failure
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Have unit tests been run against this PR? (Has there also been any additional testing?)
Related Python client changes (link commit/PR here)
Related documentation changes (link commit/PR here)
Other information:
Please check if the PR fulfills these requirements