-
Notifications
You must be signed in to change notification settings - Fork 67
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
chore(weave): Refactor and rename code to more appropriately handle builtin_object_class
not base_object_class
#3229
base: master
Are you sure you want to change the base?
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=a16822174cee2155754d9dcb918fdc18321ad335 |
object_class
not base_object_class
builtin_object_class
not base_object_class
@@ -0,0 +1,151 @@ | |||
from typing import Any, Optional, TypedDict |
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 is an edited version of the now deleted weave/trace_server/base_object_class_util.py
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.
mostly just comments and name changes
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 is just a rename of another file
> = TraceObjSchema<BaseObjectClassType<C>, C>; | ||
|
||
export const useBaseObjectInstances = <C extends BaseObjectClassRegistryKeys>( | ||
// Notice: this is still `base` object class, not `builtin` object class. |
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.
Future readers might prefer a descriptive comment rather than this notice which feels more relevant for reviewers (which I appreciate)
def model_post_init(self, __context: Any) -> None: | ||
# If set_base_object_class is provided, use it to set builtin_object_class for backwards compatibility | ||
if self.set_base_object_class is not None and self.builtin_object_class is None: | ||
self.builtin_object_class = self.set_base_object_class |
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.
neat
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.
might want to have the docs team take a peek at the docs changes, lgtm
Note: should split and deploy backend before landing front end (python extraction: #3248)
This is just a refactor, but touches many files. Specifically, in this PR: #2826 I introduced a mechanism for creating weave objects via the API for known classes. This system introduced an API param:
set_base_object_class: Optional[str] = None
. Notice thebase
word here.In nearly all the files, I used the term
base
for examplebase_object_class
,baseObjectClass
,BASE_OBJECT_REGISTRY
, etc... This is used by Leaderboards and Annotation spec definitions.However, as I am beginning to rely on this more for Scorers and Models, I realized that this was not correct. Let me take a minute to define these terms:
base_object_class
: this is the name of the first subclass of any subclass of a weave Object class.object_class
: the string of the actual class.Let's consider the following example:
In this case, the class hierarchy is:
HalucinationLLMScorer -> LLMScorer -> Scorer -> Object -> BaseModel
So, in our terms:
base_object_class
= "Scorer"object_class
= "HalucinationLLMScorer"Now,
base_object_class
is treated specially and extracted to be stored in the DB and has first-class filter support. This allows us to find all Scorers, Models, or other objects, even if they are sub classes of those "base" classes.Furthermore, in the
val
payload of the object, we have special keys:_type
= "HalucinationLLMScorer"_class_Name
= "HalucinationLLMScorer"_bases
= ["LLMScorer", "Scorer", "Object", "BaseModel"]Yes,
_type
and_class_name
are the same.So, why is this all important? Well, names are important! And the use of
base
here was not correct b/c it was intended to target the finalobject_name
of some builtin class. Therefore, this PR effectively renames all cases ofbase
tobuiltin
where appropriate such that anytimebase
appears, it really means thebase_object_class
and is no longer ambiguous.