Skip to content

Conversation

omaryashraf5
Copy link
Contributor

What does this PR do?

Modified the code in registry.py.

The key changes are:

  1. Removed the return False statement
  2. Added a warning log message that includes the object type, identifier, and provider_id for better debugging.
  3. The method now continues with the registration process instead of early returning.

@omaryashraf5 omaryashraf5 self-assigned this Sep 15, 2025
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 15, 2025
# warn if the object's providerid already exists but proceed with registration
if existing_obj and existing_obj.provider_id == obj.provider_id:
return False
logger.warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

you should warn if the provider is different -- that means an existing object is switching a provider and is perhaps a cause for concern / unanticipated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @ashwinb Updated the check

Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: this message isn't quite readable. can you make it more succinct please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@omaryashraf5 @ashwinb the warning will not be visible to anyone but the stack admin. imho, the right behavior is to reject the change with a message that the caller can see. the message should suggest unregistering first.

@omaryashraf5 omaryashraf5 changed the title Added a bug fix when registering new models fix: Added a bug fix when registering new models Sep 16, 2025
Copy link
Contributor

@ashwinb ashwinb left a comment

Choose a reason for hiding this comment

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

thx

@ashwinb ashwinb merged commit e0e2b1b into llamastack:main Sep 17, 2025
21 checks passed
mattf added a commit that referenced this pull request Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants