-
Notifications
You must be signed in to change notification settings - Fork 504
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
Prepare repo for release #356
Comments
@epwalsh Is it fine if I tackle the two remaining items in a PR? 🙂 |
@Muennighoff that would be great, go ahead. |
Great - did the first one here: #375 For splitting the components, I was thinking of moving the |
@Muennighoff separating out attention certainly makes sense from an API perspective. I think we used to have it that way, actually, but then our different block implementations became highly coupled with how attention was implemented, so we did away with that. There might be a clean way to decouple them again, but that makes me a bit nervous because at this point we need to make sure any code changes to the model do not change:
|
I see, maybe it's not worth splitting them out then? Are there other parts you want to change to be releasable? I find the current modeling code not that bad, but I would maybe clean up some of the comments and simplify a few things without changing the logic. |
@Muennighoff let's try to avoid code changes unless there's low-hanging fruit that won't have side-affects on training. I think it's still worth reorganizing + doing some clean up that you're suggesting. |
Should we close this issue? I think the 5 points are now addressed, but let me know if not & I can work on it :) |
I think we're good! |
util.py
and into their own module. (Separate torch utils #375)model.py
into submodules. (Small simplifications #377)The text was updated successfully, but these errors were encountered: