Skip to content

Fix: add model for image generation #652

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

StrongMonkey
Copy link
Contributor

obot-platform/obot#2678

Fix xai model provider that doesn't have image-generation usage.

Comment on lines 146 to 166
for usage, filters := range usageMap {
for i, model := range models.Data {
if model.Metadata == nil {
model.Metadata = make(map[string]string)
}

if len(filters) == 0 {
model.Metadata["usage"] = usage
} else {
for _, filter := range filters {
if filter(model.ID) {
model.Metadata["usage"] = usage
break
}
}
}
models.Data[i] = model
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The design of this could technically allow for one model to have the usage set to one thing, and then have it get overwritten by a later iteration if it matches the filters for a different usage. Do we want to do that? Or should we add something like this to the loop to skip models that already have a usage?

if model.Metadata["usage"] != "" {
    continue
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, not sure, I wrote this method to fix the xai model where it doesn't have its own usage. I guess we can extend to other cases. But I will add the thing you suggested for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For xai, it doesn't have usage. So we are just grouping usage based on its own name.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was just thinking for the case of reusability, since this is in the package that the other model providers depend on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the skip logic you suggested so that if it has usage then it will skip.

Copy link
Contributor

@thedadams thedadams left a comment

Choose a reason for hiding this comment

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

It looks like this logic could be achieved with the existing RewriteAllModelsWithUsage function. But your implementation seems simpler, so I am good with it.

Comment on lines 148 to 152
if model.Metadata == nil {
model.Metadata = make(map[string]string)
}

if model.Metadata["usage"] != "" {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

tiny optimization

Suggested change
if model.Metadata == nil {
model.Metadata = make(map[string]string)
}
if model.Metadata["usage"] != "" {
continue
}
if model.Metadata == nil {
model.Metadata = make(map[string]string)
} else if model.Metadata["usage"] != "" {
continue
}

@StrongMonkey StrongMonkey merged commit 78871b3 into obot-platform:main May 14, 2025
2 checks passed
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