Skip to content

Improved test cases by using paramerters#14246

Merged
AdamLouly merged 2 commits intomainfrom
adamlouly/improve_py_bindings_tests
Jan 13, 2023
Merged

Improved test cases by using paramerters#14246
AdamLouly merged 2 commits intomainfrom
adamlouly/improve_py_bindings_tests

Conversation

@AdamLouly
Copy link
Contributor

Description

Completing some missing parts of some test cases for python bindings

Motivation and Context

Some test cases like test_training_module_checkpoint and test_optimizer step were not completed before because we had no access to parameters to check if the parameters are changing after the optimizer step or that the checkpoint saved parameters remains the same.
now that we have access to the vector or parameters by exposing get_contiguous_parameters() method.
we can complete the tests.

@AdamLouly AdamLouly self-assigned this Jan 12, 2023
@AdamLouly AdamLouly requested review from askhade and pengwa January 12, 2023 00:54
optimizer = Optimizer(optimizer_file_path, model)

model.train()
old_params = model.get_contiguous_parameters()
Copy link
Contributor

Choose a reason for hiding this comment

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

old_flatten_params?

# Assert the checkpoint was saved.
assert os.path.exists(checkpoint_save_path)

# Assert the checkpoint parameters remains after saving.
Copy link
Contributor

Choose a reason for hiding this comment

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

--> remain

old_params = model.get_contiguous_parameters()

# TODO : Load checkpoint to a zeroed model and assert parameters are different.
# Assert the checkpoint was saved.
Copy link
Contributor

Choose a reason for hiding this comment

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

looks this change did complete the TODO "TODO : Load checkpoint to a zeroed model and assert parameters are different."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the change I did will test the same thing, there's no need to create a zeroed model and load it to test if the checkpoint params remain the same.

@AdamLouly
Copy link
Contributor Author

Thanks for the review

@AdamLouly AdamLouly merged commit f0555eb into main Jan 13, 2023
@AdamLouly AdamLouly deleted the adamlouly/improve_py_bindings_tests branch January 13, 2023 20:54
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