-
Notifications
You must be signed in to change notification settings - Fork 60
For issue 1575: Eliminating Manual Class Registration in Unitxt, replaced by Import Paths #1713
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
Open
dafnapension
wants to merge
4
commits into
main
Choose a base branch
from
json
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d9425b2
to
3c9f7fa
Compare
For: #1575 |
54efa18
to
137df10
Compare
83458db
to
5c835c1
Compare
1ef8d61
to
663239e
Compare
429047d
to
f3e6412
Compare
|
b0fc597
to
d576e03
Compare
ec5e524
to
ddbda04
Compare
606733f
to
4deea48
Compare
cd4c6da
to
3697c7b
Compare
1f1937a
to
bb1df84
Compare
e1a908a
to
d8a35e2
Compare
… and name, rather than snake of class name, and use _class_register only for special cases (like deprecated classes) Signed-off-by: dafnapension <[email protected]>
Signed-off-by: dafnapension <[email protected]>
Signed-off-by: elronbandel <[email protected]>
Signed-off-by: dafnapension <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
__type__
in catalog is expressed as a dict {module
: module,name
: class_name}, therefrom classes are instantiated through python's import utils.This means that if a class
c
is defined in some file that sits in pathp
, andc
is referenced in a catalog entry, and since by this PR,c
is referenced throughp
, then this reference coerces the defining filep
to stay in place in the file system. Same coercion that is induced by a line of code readingfrom p import c
. If the defining file moves in the file system, the reference in the catalog should be updated, same as any import line (as above) should be updated.Backward Compatibility:
(1) A utility,
utils/prepare_all_artifacts.py
is provided which transforms a given catalog to the new format, by running the set ofprepare
modules. Needs to be invoked once per project.(2) Also if not converted to the new format, a given catalog in the old format (where
__type__
has a string value, a snake case of the class name) can be read and worked with by the PR's code: the code translates the__type__
upon loading from the catalog to the dict format. This is effective for all__type__
that refer tounitxt
classes (classes in theunitxt/src/unitxt
directory). Yet to be developed: on-the-air translation of__type__
that refer to user-defined classes.