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

Opts cleaning #61

Merged
merged 8 commits into from May 17, 2024
Merged

Opts cleaning #61

merged 8 commits into from May 17, 2024

Conversation

TimotheeMickus
Copy link
Collaborator

closes #60

@TimotheeMickus
Copy link
Collaborator Author

@shaoxiongji this could impact docs.

@TimotheeMickus TimotheeMickus marked this pull request as ready for review March 12, 2024 09:55
@TimotheeMickus
Copy link
Collaborator Author

smoketested with hydra config on 1 GPU during test. Seems to be working properly.

Copy link
Collaborator

@Waino Waino left a comment

Choose a reason for hiding this comment

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

LGTM. A few of the maybes can be removed.

mammoth/opts.py Outdated Show resolved Hide resolved
@@ -537,10 +350,12 @@ def model_opts(parser):
group.add(
'--transformer_ff', '-transformer_ff', type=int, default=2048, help='Size of hidden transformer feed-forward'
)
# TODO is this actually in use?
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is passed around in the code. Not sure if functional, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

leaving is as is for now, ideally #56 would remove that

mammoth/opts.py Outdated Show resolved Hide resolved
@TimotheeMickus TimotheeMickus merged commit a197f6b into main May 17, 2024
2 checks passed
@TimotheeMickus TimotheeMickus deleted the fix/opts-cleanup branch May 17, 2024 06:30
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.

Opts need a major cleanup
2 participants