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

[Lazy RFC] Rename utils/ directory #1414

Closed
ebsmothers opened this issue Aug 26, 2024 · 4 comments
Closed

[Lazy RFC] Rename utils/ directory #1414

ebsmothers opened this issue Aug 26, 2024 · 4 comments

Comments

@ebsmothers
Copy link
Contributor

Why should we do this?

utils means nothing.. it is not informative. What is actually in our utils folder today? A sampling:

  • Activation checkpointing wrappers
  • Various distributed functions
  • Precision
  • Collator (?!)
  • Metric logger
  • Seed
  • Optimizer in backward hooks
  • Quantization modes

In general, when naming Python modules, we should try to find a shared purpose for all the functionality in the module. My favorite refrain is "naming is hard". But in this case, I think we can do better.

There is a clear shared purpose for all of the above "utils".. training. OK well there is utils/_generation.py.. so recipe_components. Or framework. Honestly idc.. all of these are better than utils. My actual proposal is that we should rename utils -> training because most any training framework (which ofc we are not) still needs to support inference in some capacity anyways. But if others who are more inclined to name things have strong opinions I am open to them.

Why else should we do this?

Our dependency graph. Time and time again we have some internal util we want to write (version checks on PyTorch or ao, deprecation flags for features). Such functionality should be at the bottom of our dependency tree: I should be able to gate any module import in our library on a version check if I need to. But utils is at the top of our dependency tree: it relies on our modules, our data folder, etc. We should assess whether we need all those dependencies (I claim we don't need all of them). But honestly this is reasonable.. if our training utilities need to know about our PEFT components, or need to define custom sharding strategies based on module types, it's expected that they will take a dependency on our modules folder. The point is, this makes it impossible for us to have actual utils.

Why shouldn't we do this?

Renaming utils is probably the most BC-breaking thing we can do. All our recipes just have import utils so by changing that we'll break pretty much everything. But the longer we wait to do it (and yes, we do need to do it) the more painful it will be. Not to pick on @RdoubleA but there is absolutely no reason our deprecation flag should live under the data/ directory (see #1286). But again, this is where we will be until we make this change. Better to rip the bandaid off sooner rather than later.

@pytorch pytorch deleted a comment Aug 26, 2024
@pytorch pytorch deleted a comment Aug 26, 2024
@pytorch pytorch deleted a comment Aug 26, 2024
@SalmanMohammadi
Copy link
Collaborator

wth is this spam

@SalmanMohammadi
Copy link
Collaborator

Sorry I don't mean you @ebsmothers

@RdoubleA
Copy link
Contributor

+++++1 to this. but also I do want to point out the the circular dependencies largely stems from _checkpointer which takes a dependency on models, which takes a dependency on data and modules, which takes a dependency on utils... you get the point. I don't know if simply renaming the folder will remove the cycles as opposed to actually moving certain things out and manually modifying the dependency graph.

@RdoubleA
Copy link
Contributor

RdoubleA commented Aug 27, 2024

Some thoughts on where files can go:

training

  • _checkpointing
    • constants.py
  • _device.py
  • _distributed.py
  • _profiler.py
  • activations.py
  • memory.py
  • metric_logging.py
  • pooling.py
  • precision.py
  • quantization.py
  • seed.py

generation

  • _generation.py

config

  • argparse.py

data

  • collate.py

utils

  • _version.py
  • logging.py

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

No branches or pull requests

5 participants
@ebsmothers @SalmanMohammadi @RdoubleA and others