Skip to content

Conversation

@younesbelkada
Copy link
Contributor

@younesbelkada younesbelkada commented Aug 18, 2023

What does this PR do?

As per title and to address #25077 (comment)
Let's move all third party libs (outside HF ecosystem) related utility files inside lib_integrations/, currently to the best of my knowledge bitsandbytes and deepspeed are the only 2 third party libs that we use as a plugin integration and is outside HF ecosystem

cc @sgugger and @stas00 as it touches DS related code

Let me see first if the CI passes

@younesbelkada younesbelkada changed the title [Refactor] Move third-party related utility files into lib_integrations/ folder 🚨🚨🚨 [Refactor] Move third-party related utility files into lib_integrations/ folder 🚨🚨🚨 Aug 18, 2023
@younesbelkada younesbelkada requested a review from stas00 August 18, 2023 18:15
@younesbelkada younesbelkada marked this pull request as ready for review August 18, 2023 18:15
@younesbelkada younesbelkada requested a review from sgugger August 18, 2023 18:15
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 18, 2023

The documentation is not available anymore as the PR was closed or merged.

@stas00
Copy link
Contributor

stas00 commented Aug 18, 2023

Hi Younes,

Unless I'm mistaken these would be BC breaking changes.

$ PYTHONPATH=src python -c "from transformers.deepspeed import HfDeepSpeedConfig"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'transformers.deepspeed'

Changing the doc to use the new API won't unbreak users' code.

(In general for deepspeed integration issues please tag @pacman100 who is the current maintainer of the integration.)

@stas00
Copy link
Contributor

stas00 commented Aug 18, 2023

unrelated - perhaps there could be a neater name than lib_integrations - this is awkward IMHO.

Perhaps just integrations or integration or external - in other words one word would be smoother. it's obvious that those are libs. all python packages are.

- from transformers.lib_integrations.deepspeed import HfDeepSpeedConfig
+ from transformers.integration.deepspeed import HfDeepSpeedConfig

@sgugger
Copy link
Collaborator

sgugger commented Aug 18, 2023

Yes I had mentioned integrations as well as a name for the folder. Note that we usually do not guarantee backward compatibility with imports not at the init level (anything not imported in the main init or a subfolder init is considered private), but we can keep a deepspeed module that reimports HfDeepSpeedConfig if this line is in a lot of places.

@younesbelkada
Copy link
Contributor Author

I did not used integrations as there is already a module named integrations.py in the same place. Maybe renaming lib_integrations to external would be better, WDYT?

@stas00
Copy link
Contributor

stas00 commented Aug 18, 2023

Use a single integration? Since once you add the specifics it's a clear transformers.integration.what

as in transformers.integration.deepspeed aka transformers' integration of deepspeed

@sgugger
Copy link
Collaborator

sgugger commented Aug 18, 2023

As long as the objects are re-imported, it's completely fine (and non-breaking) to change integrations.py to an integrations folder.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Not sur we need subfolders for DeepSpeed, bitandbytes and peft, they can jus be modules no?

@younesbelkada
Copy link
Contributor Author

Ah yes makes sense, modified it accordingly

@younesbelkada younesbelkada requested a review from sgugger August 21, 2023 12:57
@younesbelkada
Copy link
Contributor Author

Actually sorry, forgot to do it for peft, will do it now

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 21, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Contributor

@pacman100 pacman100 left a comment

Choose a reason for hiding this comment

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

Thank you @younesbelkada for iterating!

@younesbelkada
Copy link
Contributor Author

Thanks everyone for your reviews, regarding the large diff on the doctest file my intuition is that it got large because I added a new line that changed the alphabetical order of the file but I am not sure. (maybe @ydshieh can confirm)
I also realised we might need to keep utils/bitsandbytes.py otherwise it would be too breaking, so I reverted it back and did a similar approach than deepspeed.py

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Aug 23, 2023

The large diff is fixed with #25680 thanks to @ydshieh

@younesbelkada younesbelkada requested a review from sgugger August 23, 2023 09:54
@younesbelkada
Copy link
Contributor Author

@sgugger , ran the daily CI (with a reduced number of models tests) thanks to @ydshieh and you can see the results here: https://github.com/huggingface/transformers/actions/runs/5965296109
compared the report with the report of the same day and I don't see any surprising difference / test failure caused by this PR. Therefore I think we can merge it ! Can you please have a final look ? 🙏 Thanks!

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this!

@younesbelkada younesbelkada merged commit 4b79697 into huggingface:main Aug 25, 2023
@younesbelkada younesbelkada deleted the move-integrations branch August 25, 2023 15:13
@ydshieh ydshieh mentioned this pull request Aug 30, 2023
osanseviero added a commit that referenced this pull request Sep 1, 2023
amyeroberts pushed a commit that referenced this pull request Sep 4, 2023
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
…tions/` folder 🚨🚨🚨 (huggingface#25599)

* move deepspeed to `lib_integrations.deepspeed`

* more refactor

* oops

* fix slow tests

* Fix docs

* fix docs

* addess feedback

* address feedback

* final modifs for PEFT

* fixup

* ok now

* trigger CI

* trigger CI again

* Update docs/source/en/main_classes/deepspeed.md

Co-authored-by: Sylvain Gugger <[email protected]>

* import from `integrations`

* address feedback

* revert removal of `deepspeed` module

* revert removal of `deepspeed` module

* fix conflicts

* ooops

* oops

* add deprecation warning

* place it on the top

* put `FutureWarning`

* fix conflicts with not_doctested.txt

* add back `bitsandbytes` module with a depr warning

* fix

* fix

* fixup

* oops

* fix doctests

---------

Co-authored-by: Sylvain Gugger <[email protected]>
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
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.

5 participants