-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add SmolVLM2 #1106
Add SmolVLM2 #1106
Conversation
…nto add-smolvlm
@@ -172,6 +176,15 @@ def get_model_type( | |||
device_id=GLOBAL_DEVICE_ID, | |||
).get("ort") | |||
project_task_type = api_data.get("type", "object-detection") | |||
elif model_id in FOUNDATION_MODELS_TO_DOWNLOAD_AS_CORE: | |||
api_data = get_roboflow_model_data( |
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.
why is that needed here but wasnt needed for other core models?
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 am a little bit shaky letting that go to main
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.
if required - should be set instead of list for faster lookup as models pool grows
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.
looks good, but please explain the changes in get_model_type
why |
@capjamesg - chane will not be pushed to release if comments are not addressed. |
…nto add-smolvlm
@@ -163,7 +167,7 @@ def get_model_type( | |||
model_type=model_type, | |||
) | |||
return project_task_type, model_type | |||
|
|||
model_type = None |
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.
probably not needed modification
workflow_definition=SMOLVLM2_WORKFLOW_DEFINITION, | ||
workflow_name_in_app="smolvlm2" | ||
) | ||
# @pytest.mark.skipif( |
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.
remove commented out code
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.
looks good, please revert not needed change from inference/core/registries/roboflow.py
# some older projects do not have type field - hence defaulting | ||
model_type = api_data.get("modelType") | ||
if not model_type: |
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.
this should be adjusted
Description
This PR adds SmolVLM2 to Inference.
Type of change
How has this change been tested, please provide a testcase or example of how you tested the change?
You can test this change with the following code:
You should see a result like:
Any specific deployment considerations
N/A
Docs
A docs page has been added for this model.