-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Update optimizers.py #8920
Update optimizers.py #8920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes no effective logical change, but introduced potential bugs by removing docs string, type annotations and even entire optimization cases for some reason.
@@ -1,49 +1,33 @@ | |||
# Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert. You cannot remove the license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more about the exact bugs this implementation will cause? Or are these just projections? Readibility is certainly non-trivial and a PR doesn't need a specific logical change to be non-trivial. This code was also optimized and this was noted at the PR.
@@ -54,20 +38,18 @@ | |||
except ModuleNotFoundError: | |||
HAVE_APEX = False | |||
|
|||
HAVE_APEX_DISTRIBUTED_ADAM = False | |||
# Check for APEX distributed Adam optimizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the bool?
if hasattr(optimizer_kwargs, 'keys'): | ||
# Attempt class path resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove docstring ?
optimizer_kwargs_config = OmegaConf.create(optimizer_kwargs) | ||
optimizer_instance = hydra.utils.instantiate(optimizer_kwargs_config) # type: DictConfig | ||
optimizer_instance = hydra.utils.instantiate(optimizer_kwargs_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the need to remove type annotations ?
if optimizer_kwargs['name'] == 'auto': | ||
optimizer_params_name = "{}_params".format(optimizer_name) | ||
optimizer_kwargs.pop('name') | ||
else: | ||
optimizer_params_name = optimizer_kwargs.pop('name') | ||
|
||
# Override arguments provided in the config yaml file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
else: | ||
# If we are provided a partial class instantiation of a Config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
adam_nondist_optims = (optim.Adam, optim.AdamW) | ||
if HAVE_APEX: | ||
adam_nondist_optims += (FusedAdam,) | ||
if isinstance(optimizer, adam_nondist_optims): | ||
for group in optimizer.param_groups: | ||
for p in group['params']: | ||
state = optimizer.state[p] | ||
if len(state) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? Why remove this?
Readability is important and has utility at iterating at software in
general. A PR doesn’t need a logical change to have utility at a given
implementation. Can you say more about the bugs this implementation
introduces?
…On Sun, Apr 14, 2024 at 3:48 PM Somshubra Majumdar ***@***.***> wrote:
***@***.**** requested changes on this pull request.
This PR makes no effective logical change, but introduced potential bugs
by removing docs string, type annotations and even entire optimization
cases for some reason.
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> @@ -1,49 +1,33 @@
-# Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved.
Revert. You cannot remove the license header
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> @@ -54,20 +38,18 @@
except ModuleNotFoundError:
HAVE_APEX = False
-HAVE_APEX_DISTRIBUTED_ADAM = False
+# Check for APEX distributed Adam optimizer
Why remove the bool?
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> if hasattr(optimizer_kwargs, 'keys'):
- # Attempt class path resolution
Why remove docstring ?
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> optimizer_kwargs_config = OmegaConf.create(optimizer_kwargs)
- optimizer_instance = hydra.utils.instantiate(optimizer_kwargs_config) # type: DictConfig
+ optimizer_instance = hydra.utils.instantiate(optimizer_kwargs_config)
What's the need to remove type annotations ?
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> if optimizer_kwargs['name'] == 'auto':
optimizer_params_name = "{}_params".format(optimizer_name)
optimizer_kwargs.pop('name')
else:
optimizer_params_name = optimizer_kwargs.pop('name')
- # Override arguments provided in the config yaml file
++
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> else:
- # If we are provided a partial class instantiation of a Config,
++
------------------------------
In nemo/core/optim/optimizers.py
<#8920 (comment)>:
> adam_nondist_optims = (optim.Adam, optim.AdamW)
if HAVE_APEX:
adam_nondist_optims += (FusedAdam,)
if isinstance(optimizer, adam_nondist_optims):
for group in optimizer.param_groups:
for p in group['params']:
state = optimizer.state[p]
- if len(state) == 0:
?? Why remove this?
—
Reply to this email directly, view it on GitHub
<#8920 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFMUSBKWEOVYH7DWPZFXS6LY5J3ILAVCNFSM6AAAAABGGEFNDWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJZGU4TEMZXGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I made several refinements and optimizations to improve the efficiency, readability, and maintainability of the code: Code Structure Optimization: Consolidated imports to improve readability and reduce clutter. Grouped related code blocks together for better coherence. Error Handling Improvement: Simplified error handling by raising meaningful exceptions when encountering errors. This enhances code robustness and makes debugging easier. Efficient Data Handling: Optimized data handling by directly returning default values if the input is None, eliminating unnecessary computations and enhancing code efficiency. Simplified Logic: Simplified logic by removing redundant conditions and unnecessary checks, resulting in cleaner and more concise code. Optimized Imports: Removed redundant imports and unused dependencies to streamline the codebase and reduce overhead. Enhanced Readability: Improved code readability by using descriptive variable names and concise comments where necessary, aiding in code comprehension and maintenance. Consistent Naming Conventions: Ensured consistent naming conventions throughout the codebase for better clarity and maintainability. These changes collectively refine the codebase, making it more efficient, readable, and maintainable while preserving its functionality and purpose. Signed-off-by: hengittää <matthew.stephenson@uah.edu>
for more information, see https://pre-commit.ci
This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days. |
This PR was closed because it has been inactive for 7 days since being marked as stale. |
I made several refinements and optimizations to improve the efficiency, readability, and maintainability of the code:
Code Structure Optimization:
Consolidated imports to improve readability and reduce clutter. Grouped related code blocks together for better coherence. Error Handling Improvement:
Simplified error handling by raising meaningful exceptions when encountering errors. This enhances code robustness and makes debugging easier. Efficient Data Handling:
Optimized data handling by directly returning default values if the input is None, eliminating unnecessary computations and enhancing code efficiency. Simplified Logic:
Simplified logic by removing redundant conditions and unnecessary checks, resulting in cleaner and more concise code. Optimized Imports:
Removed redundant imports and unused dependencies to streamline the codebase and reduce overhead. Enhanced Readability:
Improved code readability by using descriptive variable names and concise comments where necessary, aiding in code comprehension and maintenance. Consistent Naming Conventions:
Ensured consistent naming conventions throughout the codebase for better clarity and maintainability.
These changes collectively refine the codebase, making it more efficient, readable, and maintainable while preserving its functionality and purpose.
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Collection: [Note which collection this PR will affect]
Changelog
Usage
# Add a code snippet demonstrating how to use this
Jenkins CI
To run Jenkins, a NeMo User with write access must comment
jenkins
on the PR.Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information