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

Share a list of weight attributes instead of a single one in TiedLayerSpec API #2035

Closed

Conversation

thomasw21
Copy link
Contributor

No description provided.

@@ -191,7 +203,7 @@ def forward(self, inputs):
self.forward_funcs = []
self.fwd_map = {}
self.tied_modules = nn.ModuleDict()
self.tied_weight_attrs = {}
self.tied_weight_attrss = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the double s a typo or intentional?

@tjruwase tjruwase requested a review from samadejacobs as a code owner July 27, 2022 14:11
@tjruwase
Copy link
Contributor

@thomasw21, thanks for the PR and apologies for the delay in reviewing. Can you please provide a clarification? It seems there are two issues being addressed in this PR: (1) supporting user-specified forward function and (2) list of weight attributes. If this is correct, I think it might be better to split into two separate PRs. What do you think? Thanks!

@thomasw21
Copy link
Contributor Author

Indeed! Sorry I've sort of dropped this as well currently, as we've been focusing on other aspects of BigScience. I'll try split the PRs when I get the chance! My bad!

@jomayeri
Copy link
Contributor

@tjruwase Are these changes we still want? If so I can revive them with the current develop branch.

@jomayeri jomayeri self-assigned this Aug 25, 2023
@tjruwase
Copy link
Contributor

@tjruwase Are these changes we still want? If so I can revive them with the current develop branch.

@thomasw21, are you still interested in this PR? Are you fine with #4216 replacing this?

@thomasw21
Copy link
Contributor Author

Hey! I'm fine with #4216 overriding this PR :D

@thomasw21 thomasw21 closed this Aug 28, 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.

4 participants