Skip to content
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

Remove HfConfig::dataset references in examples and tests #1113

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

shaahji
Copy link
Contributor

@shaahji shaahji commented Apr 25, 2024

Remove HfConfig::dataset references in examples and tests

Remove references to HFConfig::dataset from input model in all examples and tests. Moved the references to the dataset into data_configs and updated references accordingly. This change also eliminates the need for the constant input_model_data_config.

Checklist before requesting a review

  • Add unit tests for this change.
  • Make sure all tests can pass.
  • Update documents if necessary.
  • Lint and apply fixes to your code by running lintrunner -a
  • Is this a user-facing change? If yes, give a description of this change to be included in the release notes.
  • Is this PR including examples changes? If yes, please remember to update example documentation in a follow-up PR.

(Optional) Issue link

@shaahji shaahji changed the title Remove HfConfig::datasetreferences in examples and tests Remove HfConfig::dataset references in examples and tests Apr 25, 2024
jambayk
jambayk previously approved these changes Apr 25, 2024
Copy link
Contributor

@jambayk jambayk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the changes look good to me. Just want to confirm with @trajepl that will be okay/safe to remove the feature at

if self.hf_config.dataset:

Since HFConfig::dataset will be removed after this.

@jambayk jambayk dismissed their stale review April 25, 2024 23:52

Will approve again once Jiapeng gives his response on the question above.

@shaahji
Copy link
Contributor Author

shaahji commented Apr 25, 2024

Thanks, the changes look good to me. Just want to confirm with @trajepl that will be okay/safe to remove the feature at

if self.hf_config.dataset:

Since HFConfig::dataset will be removed after this.

The function get_dummy_inputs will likely just take a DataConfig as optional argument.

P.S. This is my first impression from looking at the function implementation. I am guessing the call-site for the function itself has access to the specific DataConfig in play.

jambayk
jambayk previously approved these changes Apr 26, 2024
@jambayk jambayk dismissed their stale review April 26, 2024 05:46

PR not ready yet

@trajepl
Copy link
Contributor

trajepl commented Apr 26, 2024

231d239#diff-3fbc96d0a23f27fb63d646e8949b46cf42cb0aedcfa10e8caca0f331f0a119e2
Please rebase the main branch since I moved the bert auto optimization into notebook section.

@shaahji
Copy link
Contributor Author

shaahji commented Apr 26, 2024

@trajepl , @jambayk , @guotuofeng Rebased and ready for re-re-review.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@shaahji shaahji force-pushed the shaahji/dc2 branch 2 times, most recently from 69cd0ab to ad20783 Compare April 26, 2024 19:31
Remove references to HFConfig::dataset from input model in all examples
and tests. Moved the references to the dataset into data_configs and
updated references accordingly. This change also eliminates the need for
the constant __input_model_data_config__.
Copy link
Contributor

@jambayk jambayk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@shaahji shaahji merged commit fc128ef into main Apr 26, 2024
35 checks passed
@shaahji shaahji deleted the shaahji/dc2 branch April 26, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants