Refactor artifact.py, fix an issue, identify two others #452
Conversation
| # It is likely that other parts of the code are using this cache in an | ||
| # undisciplined fashion. We should formalize this cache. | ||
| return { | ||
| # 'artifact': self.artifact, :-( |
There was a problem hiding this comment.
The whole cache is useless, because we weren't caching the actual underlying artifact.
More, if I try to add the artifact to the cache, I get exceptions.
There was a problem hiding this comment.
Maybe cache is the wrong choice of words.
| object_id = r.pop('object_id', None) | ||
| try: | ||
| # TODO: this won't be any use without 'artifact' being set | ||
| return Artifact(**cache[object_id]) |
There was a problem hiding this comment.
This was originally returning a dict which is not what the code intended to do.
| except KeyError: | ||
| pass | ||
|
|
||
| # TODO: if `file_id` is not popped but passed to the constructor of Artifact, |
There was a problem hiding this comment.
This also seems to mean we aren't keeping the file_id so we will believe we are replacing files we just "already know about.
| @@ -105,7 +106,7 @@ def retriever(self) -> LangchainRetriever: | |||
| def chain(self) -> Chain: | |||
| assert hasattr(self, 'retriever') | |||
| return RetrievalQAWithSourcesChain.from_chain_type( | |||
| llm=self.object.a, | |||
| llm=t.cast(BaseLanguageModel, self.object.artifact), | |||
There was a problem hiding this comment.
Couldn't resist adding a cast.
All the changes after this point are uninteresting. :-)
| return a | ||
|
|
||
| @property | ||
| def a(self): |
There was a problem hiding this comment.
This short-hand is used all over the code-base. Do the tests pass with that removed?
| @@ -25,7 +25,7 @@ def TransformersTrainerConfiguration(identifier: str, *args, **kwargs): | |||
| class Pipeline(Model): | |||
| @property | |||
| def pipeline(self): | |||
| return self.object.a | |||
| return self.object.artifact | |||
There was a problem hiding this comment.
Ok, I see you've replaced all of those.
There was a problem hiding this comment.
Yeah, .a is just too, too brief. And it isn't actually used in that many places.
Description
core/artifact.py got the quality makeover, exposing three issues.
Related Issue(s)
See #408
Additional Notes
I'll do a review, pointing out the key changes.