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.

Sign up for GitHub

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

Jump to bottom

[Good First Issue] [NNCF] Make NNCF common accuracy aware training code pass mypy checks #2492

Closed
vshampor opened this issue Jan 24, 2024 · 17 comments · Fixed by #2637
Closed

[Good First Issue] [NNCF] Make NNCF common accuracy aware training code pass mypy checks #2492

vshampor opened this issue Jan 24, 2024 · 17 comments · Fixed by #2637
Assignees
@anzr299
Labels
good first issue Good for newcomers

Comments

@vshampor
Copy link
Contributor

This is exactly like #2495 (see the description and tasks there), but the target code path for this one is nncf/common/accuracy_aware_training instead of nncf/common/graph.
@vshampor vshampor mentioned this issue Jan 24, 2024
Closed
@siddhant-0707
Copy link

.take

@github-actions GitHub Actions
Copy link

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@mlukasze mlukasze added the good first issue Good for newcomers label Jan 25, 2024
@tvilight4
Copy link

@vshampor missed this!! will wait for the next one!! thanks for informing!!

@vshampor
Copy link
Contributor Author

@tvilight4 I have created #2491 now, take a look.

@tvilight4
Copy link

@vshampor thanks!! got assinged for it.
will start working on the same

@p-wysocki
Copy link
Contributor

Hello @tvilight4, are you still working on that issue or can we reassign it?

@ilya-lavrenov ilya-lavrenov transferred this issue from openvinotoolkit/openvino Feb 20, 2024
@siddhant-0707 siddhant-0707 mentioned this issue Feb 26, 2024
Open
@p-wysocki p-wysocki moved this from Assigned to In Review in Good first issues Mar 12, 2024
@p-wysocki
Copy link
Contributor

@vshampor could you please either link the PR to the development section of the issue or grant me some permissions in NNCF repo to do this myself?

image

I'd prefer the latter since I'm keeping an eye on all GFIs weekly.

@anzr299
Copy link
Contributor

anzr299 commented Apr 9, 2024

@p-wysocki Is this issue being actively worked on? If not, I'm down to work on it!

@p-wysocki
Copy link
Contributor

Hi @anzr299, sure! The thing is this issue is in NNCF repository and I do not have correct access rights to assign you. @vshampor could you please do it?

For now let's agree the task is yours, feel free to start working on it and ask questions. We'll figure out the formalities in the background. :)

@anzr299
Copy link
Contributor

anzr299 commented Apr 9, 2024

Sure, thank you very much! I have already started working on it.

@p-wysocki p-wysocki moved this from In Review to Assigned in Good first issues Apr 9, 2024
@anzr299
Copy link
Contributor

anzr299 commented Apr 15, 2024

Hi @p-wysocki, what is the update regarding the assignment? I was off for some time due to my dissertation submission. I am resuming work on this issue now.

@vshampor vshampor assigned anzr299 and unassigned siddhant-0707 Apr 15, 2024
@p-wysocki
Copy link
Contributor

Done, please let us know if you have any questions @anzr299. :)

@anzr299
Copy link
Contributor

anzr299 commented Apr 15, 2024

Sure, I am following the current types that are mentioned in some files. For arguments with kwargs and args, I am planning on using "...", is that fine?

@p-wysocki
Copy link
Contributor

I found some good tips here, I think you can pick one you'd like to use and it's going to be fine - worst case scenario we'll change it.

@anzr299
Copy link
Contributor

anzr299 commented Apr 15, 2024

Also, I was wondering if the the | method would be preferred over the Optional() method to denote an option between type and None since the latest updates suggest a movement towards the | direction but the current NNCF code mostly uses optional().

@anzr299
Copy link
Contributor

anzr299 commented Apr 16, 2024

I am facing a problem in the following section of training_loop.py:
image
Mypy is giving an error saying the CompressionScheduler doesn't contain current_sparsity_level and current_pruning_level. A similar problem exists in the _interpolate_compression_step_update method for runner.maximal_accuracy_drop.

@anzr299 anzr299 mentioned this issue Apr 17, 2024
Merged
@anzr299
Copy link
Contributor

anzr299 commented Apr 17, 2024

@p-wysocki @vshampor I've created a PR #2637

@p-wysocki p-wysocki moved this from Assigned to In Review in Good first issues Apr 17, 2024
vshampor pushed a commit that referenced this issue Apr 25, 2024
@anzr299
Make NNCF common accuracy aware training code pass mypy checks (#2637)
17a5b65 
## Changes:
mypy checks for accuracy_aware_training pass. 

type: ignore is used in some places due to one of the following reasons:
1. Using Generic type causes a mismatch between some instances and their assignments which causes mypy to throw an error. Bounding does not solve the problem either.
2. Other issues related to the use of TypeVar such as bounding OptimizerType, LRSchedulerType, and TensorboardWriterType.
3. Error related to named arguments not being defined in `self._train_epoch_fn`
4. attribute not available

Extra Changes:
1. The argument type and return type of `configure_accuracy_aware_paths` were changed to include pathlib.Path.
2. **kwargs was removed from abstract method `initialize_training_loop_fns` due to mypy bug regarding a mismatch of the function type.

## Reasons for changes:
Passing mypy checks in issue #2492 

## Related tickets:  
Closes #2492
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@anzr299 anzr299

Labels
good first issue Good for newcomers
Projects
Good first issues
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make NNCF common accuracy aware training code pass mypy checks
6 participants
@siddhant-0707 @vshampor @anzr299 @mlukasze @p-wysocki @tvilight4