Update optimizers.py
State: closed | Pull request: yes
Labels: name: core

name: 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 
- Add specific line by line info of high level changes in this PR.

# Usage
* You can potentially add a usage example below

```python
# 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**:
- [ ] Make sure you read and followed [Contributor guidelines](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md)
- [ ] Did you write any new necessary tests?
- [ ] Did you add or update any necessary documentation?
- [ ] Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
  - [ ] Reviewer: Does the PR have correct import guards for all optional libraries?
  
**PR Type**:
- [ ] New Feature
- [ ] Bugfix
- [ ] Documentation

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](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md) contains specific people who can review PRs to various areas.

# Additional Information
* Related to # (issue)


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
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564677379>:
>
> > @@ -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
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564679098>:
>
> > @@ -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
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564680625>:
>
> >      if hasattr(optimizer_kwargs, 'keys'):
> -        # Attempt class path resolution
>
> Why remove docstring ?
> ------------------------------
>
> In nemo/core/optim/optimizers.py
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564681391>:
>
> >              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
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564682045>:
>
> >              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
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564682537>:
>
> >              else:
> -                # If we are provided a partial class instantiation of a Config,
>
> ++
> ------------------------------
>
> In nemo/core/optim/optimizers.py
> <https://github.com/NVIDIA/NeMo/pull/8920#discussion_r1564683803>:
>
> >      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
> <https://github.com/NVIDIA/NeMo/pull/8920#pullrequestreview-1999592376>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BFMUSBKWEOVYH7DWPZFXS6LY5J3ILAVCNFSM6AAAAABGGEFNDWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJZGU4TEMZXGY>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>


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.