Return more information in create_commit output #1066
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Thanks for the PR, just left a few comments on docstrings/type hints.
On the breaking change, we can leave with it in Transformers. I'd be more concerned about downstream libraries that may be using this function already. It's quite new, so one can argue it's still experimental, but that may still affect users negatively (and we do know from experience that some do not put pins in their setup.py)
Would be keen to hear other's thoughts!
|
FYI, this PR follows an internal discussion we had (see on slack -private internal link-). The hope is that the return value is never used but I guess we'll see |
Codecov ReportBase: 84.16% // Head: 84.23% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1066 +/- ##
==========================================
+ Coverage 84.16% 84.23% +0.06%
==========================================
Files 40 40
Lines 4087 4105 +18
==========================================
+ Hits 3440 3458 +18
Misses 647 647
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
The slack link does not open anywhere for me FYI. There is really no need to tag me in a PR if my input is unwanted and the discussion is held privately without me, I am very happy to leave the maintenance of the integration of the Hub in Transformers to someone who has a seat at the table of those discussions instead :-) |
|
we can share the main parts of the brainstorming convo with you @sgugger (or directly here) but rest assured it was not deliberate to exclude you, quite the contrary 😂 |
| revision = self.pr_revision | ||
| if revision is not None: | ||
| return int(revision.split("/")[-1]) | ||
|
|
There was a problem hiding this comment.
Nit: Maybe consider adding a __str__ method for a nice string representation
There was a problem hiding this comment.
@osanseviero Thanks for the comment , I discovered that a good way to handle properties in dataclasses is to use the __post_init__ method and field(init=False) in attribute definition. This way we don't use @property and the dataclass is fully aware of the computed attributes as if they were normal attributes.
See https://docs.python.org/3.10/library/dataclasses.html#post-init-processing and 475336f.
Fix #1037.
Now
create_commitreturns aCommitInfodata structure containing commit hash, commit url, commit message, commit description and eventually pr url. This way the return type is more consistent than returning onlypr_url(string) orNonedepending on thecreate_prparameter.The hope is that
create_commitis already used in downstream libraries but the return value is not. Better to change it now than later.@sgugger there is one implication in
transformershere. It only changes a logger message so it's not critical and can be quite easily changed.Note:
upload_file,upload_folderandpush_to_hubreturn types have not been changed as they were not returning the pr_url but a URL to either a file or a folder.