Skip to content

Conversation

@darshil929
Copy link
Contributor

Changes

  • Added support for ord=0 in tf_linalg.py's norm function
  • Fixed the keepdims parameter for nuclear norm case
  • Added tests that check norm with keepdims=True for both CPU and GPU classes
  • Added tests for lstsq function with multiple right-hand sides (rank-2 tensors)

Reason for changes

Right now TensorFlow tensors don't work with nncf.Tensor, which is blocking issue #3041 from being done. This PR adds TensorFlow backend support so we can use TensorFlow tensors with NNCF operations. I'm continuing the work from PR #3106 but fixing some things reviewers pointed out and adding stuff that was missing.

Related tickets

#3041
#3106

Tests

  • Added a test_norm_keepdims function to both CPU and GPU classes to check if keepdims works right
  • Added test_lstsq_rank2 to make sure least squares works with multiple columns
  • Note: I've added the requested tests, but encountered environment setup issues that prevented me from running them successfully. The error appears to be related to function signature registration in the TensorFlow backend integration, which I think is beyond the scope of my changes

@darshil929 darshil929 requested a review from a team as a code owner March 13, 2025 18:06
@github-actions github-actions bot added documentation Improvements or additions to documentation NNCF TF Pull requests that updates NNCF TensorFlow labels Mar 13, 2025
@darshil929 darshil929 marked this pull request as draft March 13, 2025 18:12
@darshil929 darshil929 marked this pull request as ready for review March 13, 2025 18:14
@alexsu52 alexsu52 requested review from alexsu52 and kshpv March 14, 2025 06:50
@github-actions github-actions bot added the API Public API-impacting changes label Mar 14, 2025
@darshil929
Copy link
Contributor Author

@kshpv

I've implemented the suggested change to the save_file.register decorator. I've also fixed string literals in exceptions in tf_linalg.py and tf_numeric.py to address the pre-commit linting issues that were causing CI failures. The PR should be ready for review now.

@kshpv
Copy link

kshpv commented Mar 17, 2025

@kshpv

I've implemented the suggested change to the save_file.register decorator. I've also fixed string literals in exceptions in tf_linalg.py and tf_numeric.py to address the pre-commit linting issues that were causing CI failures. The PR should be ready for review now.

Hi @darshil929
Could you please fix precommit issues at first?

…ersion changes, and improve tensor device handling
@darshil929
Copy link
Contributor Author

@kshpv
I've implemented the suggested change to the save_file.register decorator. I've also fixed string literals in exceptions in tf_linalg.py and tf_numeric.py to address the pre-commit linting issues that were causing CI failures. The PR should be ready for review now.

Hi @darshil929 Could you please fix precommit issues at first?

I'm analyzing the precommit issues now and will get them fixed as soon as possible.

@darshil929
Copy link
Contributor Author

Am I supposed to create a new PR to complete the work on issue #3041 or it should be done in the current PR?

It would be easier for reviewers to split the work into parts. I suggest creating a new PR

Hi @kshpv
I'm currently working on preparing my proposal draft for GSOC project 14 - "Accelerating Inference of NNCF-Compressed LLMs with Triton," for OpenVINO-NNCF. I'll raise the PR for issue #3041 as soon as my proposal work is completed, which should be within a couple of days. I would really appreciate a review of my proposal and suggestions for improvement. What would be the best way to share my Google Doc with you? Would you prefer I send it to your email, or should I contact you through Discord instead?
Thank you for your time and guidance.

You can send your proposal to alexander.suslov@intel.com

Sure sir, I am working on a example-prototype to add as a part of my proposal and demonstrate my understandings which will take a little time to complete. As soon as it is done, I will email you the draft.

@kshpv
Copy link

kshpv commented Apr 4, 2025

@kshpv

Sir, I've made some minor variable name changes and re-applied tf.identity() to the functions that were facing device preservation issues initially to maintain consistency in code. Sorry for the delay.

Could you check that adding tf.identity() resolves device issues? For all tests where it resolves, please remove the skipping device check. If it does not resolve, then please roll back adding tf.identity()

@darshil929
Copy link
Contributor Author

@kshpv
Sir, I've made some minor variable name changes and re-applied tf.identity() to the functions that were facing device preservation issues initially to maintain consistency in code. Sorry for the delay.

Could you check that adding tf.identity() resolves device issues? For all tests where it resolves, please remove the skipping device check. If it does not resolve, then please roll back adding tf.identity()

Sir, I have made the required changes.

@darshil929
Copy link
Contributor Author

Hi @alexsu52
Sir, I've implemented comprehensive test coverage for the norm function as requested:

  • Added tests for all norm types on tensors from 1D to 4D
  • Fixed the bug with nuclear norm and keepdims parameter for higher dimensions
  • Added tests for ord=0 implementation
  • Included edge case tests for empty tensors and extreme values (NaN, Inf)

All tests now pass. Could you please review when you have a chance.
Thank you!

@darshil929 darshil929 requested a review from alexsu52 April 25, 2025 05:17
@github-actions github-actions bot removed the API Public API-impacting changes label Apr 28, 2025
@darshil929
Copy link
Contributor Author

Hi @alexsu52

Sir, I've successfully moved norm tests to the TemplateTestNNCFTensorOperators class. All tests are running perfectly on the TensorFlow backend.

However, there are some failures on PyTorch and NumPy backends.

PyTorch backend failures:
Pasted image 20250504022947

NumPy backend failures:
Pasted image 20250504023028

Could you please advice if I should modify the backend implementations to handle these edge cases?
Or is there another approach you'd recommend?

Thank you!

@darshil929 darshil929 requested a review from alexsu52 May 3, 2025 21:33
@alexsu52
Copy link

alexsu52 commented May 4, 2025

Hi @alexsu52

Sir, I've successfully moved norm tests to the TemplateTestNNCFTensorOperators class. All tests are running perfectly on the TensorFlow backend.

However, there are some failures on PyTorch and NumPy backends.

PyTorch backend failures: Pasted image 20250504022947

NumPy backend failures: Pasted image 20250504023028

Could you please advice if I should modify the backend implementations to handle these edge cases? Or is there another approach you'd recommend?

Thank you!

Hi @darshil929,

Thank you for the contribution!

Please, update tests regarding the description of the norm function https://github.com/alexsu52/nncf/blob/develop/nncf/tensor/functions/linalg.py#L19. For example, the test test_norm_order_zero failed calling result = fns.linalg.norm(tensor_2d, ord=0), but the norm function does not supports ord=0 for 2d tensor https://github.com/alexsu52/nncf/blob/develop/nncf/tensor/functions/linalg.py#L38.

Some insights, numpy behavior is the default behavior unless otherwise noted. If you find a bug in the numpy, pytorch or openvino backends, please feel free to fix it. Please also keep the function description up to date.

Copy link

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

Please, fix the pre-commit tests.

@darshil929
Copy link
Contributor Author

Sir,

I've updated the norm tests to align with the documented behavior in linalg.py where ord=0 is officially supported only for vectors, not matrices. The tests now properly validate vector norm behavior while maintaining compatibility across all backends (TensorFlow, PyTorch, and NumPy).

I've also preserved the TensorFlow implementation's extended capabilities for ord=0

All tests are now passing. Could you please review these changes?

Thanks!

@darshil929 darshil929 requested a review from alexsu52 May 5, 2025 19:27
Copy link

@alexsu52 alexsu52 left a comment

Choose a reason for hiding this comment

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

LGTM

@alexsu52 alexsu52 merged commit d8f0897 into openvinotoolkit:develop May 6, 2025
19 checks passed
@darshil929 darshil929 deleted the add-tensorflow-tensor-support branch May 6, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation NNCF TF Pull requests that updates NNCF TensorFlow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants