Skip to content

Conversation

pbontrager
Copy link
Contributor

Use lazy imports for modules so that you only import the the classes you actually use. For each submodule like actors, controller, data, etc you define the public apis at the top and then define a getattr method to import them only when requested. This will make imports faster and avoid, avoid circular dependencies, and make optional dependencies easier to manage. At the app level, users would be expected to import

from forge.actor import Policy or from forge.data import collate_packed.

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 26, 2025
Copy link
Member

@joecummings joecummings left a comment

Choose a reason for hiding this comment

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

Please give me until tomorrow to look at this

Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

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

This is an abomination lol. While I definitely appreciate the motivation, I worry that this is introducing a maintenance nightmare down the line

from .policy import PolicyRouter

return PolicyRouter
raise AttributeError("fmodule {__name__} has no attribute {name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise AttributeError("fmodule {__name__} has no attribute {name}")
raise AttributeError(f"module {__name__} has no attribute {name}")

@pbontrager
Copy link
Contributor Author

While I definitely appreciate the motivation, I worry that this is introducing a maintenance nightmare down the line

How is this harder to maintain than what we had before? It's just more verbose, not more complex. I can implement the getattr method to grab the imports from a dictionary so future updates are only one line, but you said it looked too complex.

@ebsmothers
Copy link
Contributor

Oh if we're choosing between the two options I would prefer the other one. With this approach every new API added needs a corresponding else block in an __init__ file.. seems like a very obvious footgun for contributors.

While I definitely appreciate the motivation, I worry that this is introducing a maintenance nightmare down the line

How is this harder to maintain than what we had before? It's just more verbose, not more complex. I can implement the getattr method to grab the imports from a dictionary so future updates are only one line, but you said it looked too complex.

@joecummings
Copy link
Member

See #83

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants