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

Helper function to reload a quantized state_dict #164

Closed
wants to merge 1 commit into from
Closed

Helper function to reload a quantized state_dict #164

wants to merge 1 commit into from

Conversation

ManoBharathi93
Copy link

issue number: #162

@ManoBharathi93 ManoBharathi93 requested a review from dacorvo as a code owner April 11, 2024 16:46
@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

@ManoBharathi93 your pull-request is not exactly aligned with the contributions guidelines.
In particular, please try to use conventional commits.
Regarding your tests, you simply duplicated the existing quantize tests without calling the newly introduced function, hence not verifying it works.

What I suggest for requantize tests is that you simply extract test_serialize_quantized_mlp from test_quantize_mlp.py and put it in a new test_requantize_mlp.py file.
Then, change the two lines at 123-124 to use your new helper.

@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 12, 2024

@dacorvo I have loaded quantized state_dict through requantize helper to model. but fails when it run in cuda.

Can i skip cuda using pytest.mark.skip_device ? coz it's passes the test case on cpu.
``

================================================================================================ short test summary info ================================================================================================
FAILED test/model/test_requantize_mlp.py::test_serialize_quantized_mlp[cuda-safetensors-fp16-w-qint8] - RuntimeError: Expected all tensors to be on the same device, but found at least two devices, cpu and cuda:0! (when checking argument for argument other in method wrapper_CUDA__equal)
FAILED test/model/test_requantize_mlp.py::test_serialize_quantized_mlp[cuda-safetensors-fp32-w-qint8] - RuntimeError: Expected all tensors to be on the same device, but found at least two devices, cpu and cuda:0! (when checking argument for argument other in method wrapper_CUDA__equal)
================================================================================== 2 failed, 47 passed, 7 skipped, 3 warnings in 2.71s ==================================================================================

``

@ManoBharathi93
Copy link
Author

@dacorvo , can you check my changes please.

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

Please rebase your branch, then make sure it follows the contributing guidelines: the CI fails even before running the tests.
If you have trouble rebasing your branch, please look at the instructions in the guidenies and let me know if something is not clear.

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

Also, as I said before please do not cc all the tests from test_quantize_mlp.py in your new test file. This increases the codebase without any benefit.

@ManoBharathi93
Copy link
Author

Also, as I said before please do not cc all the tests from test_quantize_mlp.py in your new test file. This increases the codebase without any benefit.

Okay I will modify in the new commit

@ManoBharathi93
Copy link
Author

@dacorvo, Added test for MLP and serialize_quantized_mlp only and removed rest of the tests
image

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

Commit messages:
🚩 added_requantize
🚩 Add: Helper to reload a quantized state_dict
🚩 add:helper to requantize
✅ fix:style corrections
Error: 🚫 According to the conventional-commits specification, some of the commit messages are not valid.

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

If you just want to reword your commits, rebase iteratively from the root of your branch:

$ git rebase -i c75cd06

In the editor, select r or reword in front of the guilty commits, save and exit.
Now, rewrite each message when prompted.
Once done:

$ git push -f origin reload_quantized_state_dict

@ManoBharathi93
Copy link
Author

Commit messages: 🚩 added_requantize 🚩 Add: Helper to reload a quantized state_dict 🚩 add:helper to requantize ✅ fix:style corrections Error: 🚫 According to the conventional-commits specification, some of the commit messages are not valid.

It means, red flagged commits needs to rename as fix: added_requantize,... ?

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

They should all start with an allowed prefix, whose list is here:
416db04

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

Alternatively, you can squash all your commits into one clean commit an rephrase it to feat: add requantize helper or something like that (just select s on the left while rebasing).

@ManoBharathi93
Copy link
Author

They should all start with an allowed prefix, whose list is here: 416db04

Then in my case all should be prefix with fix: ..
image

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

I see this may be the first time you use this git feature. As I said, you could just squash them all (select s), and use a single commit message. Anyway, with reword what you edited on the first page will be ignored unfortunately, and git will ask you again for each commit (because on the first page it is just the commit titles).

@ManoBharathi93
Copy link
Author

feat: add requantize helpe

Yes I was my first time with this git features. I was squash as you said, but it deletes all my commits and my head points to this commit c75cd now and
image

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

$ git rebase --continue

@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 12, 2024

git rebase --continue

git rebase --continue
error: cannot 'squash' without a previous commit
error: please fix this using 'git rebase --edit-todo'.

image

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 12, 2024

ah yes, the first one must stay if you squash (p for the first one, s for the others)
You can alwaus git rebase --abort and restart if you want

@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 12, 2024

@dacorvo , Thanks for your patience and helping me, it takes some time to understand for me.. Now kindly check this PR.

@ManoBharathi93 ManoBharathi93 requested a review from dacorvo April 13, 2024 00:00
@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 15, 2024

@dacorvo do I missed something ? I have been waiting for your comments..test cases passed

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 15, 2024

@dacorvo do I missed something ? I have been waiting for your comments..test cases passed

Patience is the key ... you must understand that:

  • this is an open-source project,
  • I have many other things to do.

Also, please credit @calmitchell617 in your commit message because you basically cced his branch.

@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 15, 2024

Credits:@calmitchell617 with help of his work I made this PR Thanks 😊 .

@ManoBharathi93
Copy link
Author

@calmitchell617 work has to be merged since I reuse his code with some extra test case I am closing this pr.

@dacorvo
Copy link
Collaborator

dacorvo commented Apr 15, 2024

@ManoBharathi93 thank you for acknowledging this. I have other issues that require help if you want to contribute.

@ManoBharathi93
Copy link
Author

ManoBharathi93 commented Apr 15, 2024

@ManoBharathi93 thank you for acknowledging this. I have other issues that require help if you want to contribute.

😊 absolutely.

@ManoBharathi93 ManoBharathi93 deleted the reload_quantized_state_dict branch April 15, 2024 10:02
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.

2 participants