-
Notifications
You must be signed in to change notification settings - Fork 197
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
trulens-legacy #1345
trulens-legacy #1345
Conversation
return _MovedClass | ||
|
||
|
||
def moved( |
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 users have both trulens_eval installed already, and also installed new version trulens without uninstalling trulens_eval what would happen?
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.
Will try to enforce this with package constraints with this idea:
- Rename the
trulens-legacy
package totrulens_eval
, starting with version 1.0.0 which is greater than existing trulens_eval version. - In
trulens_eval
will now have requirements to install the requiredtrulens-*
packages. - With new
trulens
releases, the constraints intrulens_eval
will have to be updated to keep track (or at least occasionally). Note that from discussions with @sfc-gh-chu , the plan is to havetrulens
package track the version oftrulens-core
.
This should cover the cases where people are still using and installing trulens_eval
. To help with the migration to trulens
we can suggest a two stage migration process:
- We ask users to first replace their
install trulens_eval
withinstall trulens
or merely upgrade theirtrulens_eval
to this legacy version. As we added a constraint totrulens
to install the legacytrulens_eval
we can make sure the backwards compatibilitytrulens_eval
replaces their oldtrulens_eval
installation either way (whether theyinstall trulens
orinstall --upgrade trulens_eval
. - The user can then observe deprecation warnings from their uses of
trulens_eval
to migrate the rest of their code so it does not refer totrulens_eval
anywhere. Hopefully they do this before we remove the legacy package.
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.
@sfc-gh-chu can you review this idea and check that the implementation of it in the PR is correct?
src/legacy/trulens_eval/__init__.py
Outdated
"AzureOpenAI", | ||
"OpenAI", | ||
"Langchain", | ||
"LiteLLM", |
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.
What would happen to this if those imports like Bedrock
is not present, would it give a silent error? or do we create this list based on the conditional imports?
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.
Same as before, the optional imports mechanism will make Bedrock a dummy class.
deprecation_utils.packages_dep_warn("trulens_eval.database") | ||
|
||
from trulens.core.database import * | ||
from trulens.core.database import base |
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.
are lines 13-19 necessary given 12?
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.
Yes but it looks like line 12 does nothing. import *
only does anything if __all__
is defined in the module we are importing from. Removed line 12.
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.
Also adding a note explaining this.
# NOTE: This file had contents in trulens_eval none were public or aliases. | ||
# Because of that, this backwards compatibility module is empty. | ||
|
||
from trulens.core.database.migrations import * |
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'm confused what's going on here, in lines 12-13 you said it's empty, so what are you importing here?
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 think it was just a copy and paste to capture __all__
exports but didn't remove it after realizing there was no __all__
. However, I'm leaving this file in place because I want to also make the non __all__
names also backwards compatible in the next PR.
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'm deleting the
import *
, but leaving the file in place is what I meant.
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.
Also adding a note explaining this.
* split from larger pr * fix duplicates in makefile * remove unneeded * nits * notes * rename trulens-legacy to trulens_eval * addressing prs * addressing pr comments * another one * relock * relock * fix pr notes * renamed
Description
Creates a new version of the
trulens_eval
package for backwards compatibility during a deprecation period.trulens_eval.*
names defined in__init__.py
files for backwards compatibility to be deprecated later. Until then, a warning is issued of an old module is imported or a class or function imported via the old name is invoked. This is tested bytest_deprecation.py
which was recently added.Example warning when importing from
trulens_eval
Example warning when using something imported from
trulens_eval
:Tru(...)
trulens_eval
behaved with respect to optional imports in the past. However, in many cases, the packages that need to be installed have been changed to the optionaltrulens-*
packages instead of what was is now these packages' dependencies.Not covered
This does not cover all of the old trulens_eval names, only the exported ones. Will cover the rest once another PR that has the tests for the other names gets merged: API dump and deeper trulens_eval compatibility test #1309
Documentation changes. These will be in another PR.
Other details good to know for developers
Please include any other details of this change useful for TruLens developers.
Type of change
not work as expected)