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

Fix for e2e tests #927

Merged
merged 11 commits into from
Nov 22, 2024
Merged

Fix for e2e tests #927

merged 11 commits into from
Nov 22, 2024

Conversation

horheynm
Copy link
Collaborator

@horheynm horheynm commented Nov 19, 2024

SUMMARY:
Issue was caused by trying to save the recipe on modify_save_pretrained. This runs after the session is reset, so no recipe to save.

With SparseAutoModelForCausalLM before the commit, in the same test, recipe is not saved. It is saved in the save_model_and_recipe, which runs after oneshot. We dont do it here anymore because 1. if output path is not provided it doesnt save; 2. if we add again, there will be double save

[Notes]
Another way to fix this is to not to reset the session. This allows us to load and save the recipe. pre-commited SparseAutoModelForCausalLM state does not save the recipe.

TEST PLAN:
Checked that the output from oneshot is the same and pass the e2e tests.

Copy link

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

@horheynm horheynm changed the title fix Fix for e2e tests Nov 19, 2024
Copy link
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

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

Why is the session being reset in these cases and not our example cases? I'm just surprised we didn't see it then

@horheynm
Copy link
Collaborator Author

@dsikka
In general the session should be reset if you want a clean state - memoryless start.
Dont reset, for example, we run consecutive optimizations (ex. oneshot then finetune).

Here we can change to not to reset, but saving/not saving doesnt affect the test

Copy link
Collaborator

@dsikka dsikka left a comment

Choose a reason for hiding this comment

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

Could we update such that we're saving the recipe now please?
It's informative to have that for the e2e cases.

@horheynm
Copy link
Collaborator Author

@dsikka
The recipe now should be saved.
FYI for this e2e tests, saving recipes do not change the outcome so it wont be informative.

@dsikka
Copy link
Collaborator

dsikka commented Nov 21, 2024

@dsikka The recipe now should be saved. FYI for this e2e tests, saving recipes do not change the outcome so it wont be informative.

We push the outputs and refer to them frequently. Please look at the complete test

@horheynm
Copy link
Collaborator Author

@dsikka The recipe now should be saved. FYI for this e2e tests, saving recipes do not change the outcome so it wont be informative.

We push the outputs and refer to them frequently. Please look at the complete test

If we were referring to it frequently, we should have known recipe wasnt being pushed previously.
I can add recipe creation check on the test in the follow up pr.

dsikka
dsikka previously approved these changes Nov 21, 2024
rahul-tuli
rahul-tuli previously approved these changes Nov 21, 2024
@horheynm
Copy link
Collaborator Author

dont merge yet please, not resetting session, so lifecycle is finializing more than once, which raises error.

@horheynm horheynm dismissed stale reviews from rahul-tuli and dsikka via 704f11c November 21, 2024 16:58
@horheynm horheynm force-pushed the sparseautomodel-removal-followup branch from 704f11c to a0c49e4 Compare November 21, 2024 17:00
@horheynm horheynm force-pushed the sparseautomodel-removal-followup branch from a0c49e4 to 5cd4c11 Compare November 21, 2024 17:02
@horheynm
Copy link
Collaborator Author

horheynm commented Nov 22, 2024

Follow up pr to check the contexts of the files, and to make sure recipes exist in the session is here
Will need this pr to land first

#929

@mgoin mgoin merged commit 19027b2 into main Nov 22, 2024
6 of 7 checks passed
@mgoin mgoin deleted the sparseautomodel-removal-followup branch November 22, 2024 14:01
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.

6 participants