Add control over tolerance for failed amortized computations #64
Conversation
facebook-github-bot
added
the
CLA Signed
distributed_shampoo/utils/tests/shampoo_preconditioner_list_test.py
Outdated
Show resolved
Hide resolved
distributed_shampoo/utils/tests/shampoo_preconditioner_list_test.py
Outdated
Show resolved
Hide resolved
distributed_shampoo/utils/tests/shampoo_preconditioner_list_test.py
Outdated
Show resolved
Hide resolved
distributed_shampoo/utils/tests/shampoo_preconditioner_list_test.py
Outdated
Show resolved
Hide resolved
|
@tsunghsienlee has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@tsunghsienlee merged this pull request in 0921f45. |
| class ShampooPreconditionerConfigTest( | ||
| AbstractPreconditionerConfigTest.PreconditionerConfigTest[ | ||
| Type[ShampooPreconditionerConfig] | ||
| ] | ||
| ): | ||
| def _get_preconditioner_config_type( | ||
| self, | ||
| ) -> Type[ShampooPreconditionerConfig]: | ||
| return ShampooPreconditionerConfig | ||
|
|
||
|
|
||
| class EigenvalueCorrectedShampooPreconditionerConfigTest( | ||
| AbstractPreconditionerConfigTest.PreconditionerConfigTest[ | ||
| Type[EigenvalueCorrectedShampooPreconditionerConfig] | ||
| ] | ||
| ): | ||
| def _get_preconditioner_config_type( | ||
| self, | ||
| ) -> Type[EigenvalueCorrectedShampooPreconditionerConfig]: | ||
| return EigenvalueCorrectedShampooPreconditionerConfig |
There was a problem hiding this comment.
Hi @runame ,
It seems these two tests do not discovered by the test discovery; if you check the CI before and after this pull request, the total number of tests ran does not change, and I believe this is due to those two classes here are still considered as abstract class so they don't get instantiated at all.
There was a problem hiding this comment.
I think the two tests are actually discovered and run by the CI. Note that the "before" CI does not actually reflect the state before the two tests are added, since it is merely the PR that was created before it (#63 before #64). See the commit history for the actual order in which the PRs were merged. Now we can compare the CI before and after the two tests were added and see that the number of tests increased from 23 to 25.
Finally, I also verified locally that commenting out the two tests results in two less tests being run.
There was a problem hiding this comment.
I see. I check the commit history, and you are right the two tests are discovered in here.
Now it seems the issue is happened in the the Meta internal test discovery could not find those two tests so I was wondering the current setup is the culprit. If I run the shampoo_types_test.py in Meta internal, it will only discover 5 test cases, but it should be 7 test cases.
We might need a different setup to accommodate this unfortunately.
There was a problem hiding this comment.
Oh interesting. Do you have any idea why the tests are not discovered internally?
There was a problem hiding this comment.
Generic is the reason because if we don't use that, it will be discovered. However, how and why are something I don't know, I will create a minimum example with Generic to verify this.
There was a problem hiding this comment.
#72 is a better design to resolve this but we might want to figure this out in the future for curiosity sake.
Summary: Current tests on the configs with subclasses relied on explicit instantiating subclasses to test it. There are some limitations on this approach: 1. It is hard to catch newly added subclasses. 2. Due to some unknown interactions with `typing.Generic`, [the current `buck` test discovery mechanism is not able to discover the tests in it](facebookresearch#64 (comment)). To resolve this, this diff refactors those tests with [`type.__subclasses()`](https://docs.python.org/3/reference/datamodel.html#type.__subclasses__) to tests the configs with subclasses. Differential Revision: D67652761
) Summary: Current tests on the configs with subclasses relied on explicit instantiating subclasses to test it. There are some limitations on this approach: 1. It is hard to catch newly added subclasses. 2. Due to some unknown interactions with `typing.Generic`, [the current `buck` test discovery mechanism is not able to discover the tests in it](facebookresearch#64 (comment)). To resolve this, this diff refactors those tests with [`type.__subclasses()`](https://docs.python.org/3/reference/datamodel.html#type.__subclasses__) to tests the configs with subclasses. Reviewed By: anana10c Differential Revision: D67652761
) Summary: Current tests on the configs with subclasses relied on explicit instantiating subclasses to test it. There are some limitations on this approach: 1. It is hard to catch newly added subclasses. 2. Due to some unknown interactions with `typing.Generic`, [the current `buck` test discovery mechanism is not able to discover the tests in it](facebookresearch#64 (comment)). To resolve this, this diff refactors those tests with [`type.__subclasses()`](https://docs.python.org/3/reference/datamodel.html#type.__subclasses__) to tests the configs with subclasses. Reviewed By: anana10c Differential Revision: D67652761
Summary: Pull Request resolved: #72 Current tests on the configs with subclasses relied on explicit instantiating subclasses to test it. There are some limitations on this approach: 1. It is hard to catch newly added subclasses. 2. Due to some unknown interactions with `typing.Generic`, [the current `buck` test discovery mechanism is not able to discover the tests in it](#64 (comment)). To resolve this, this diff refactors those tests with [`type.__subclasses__`](https://docs.python.org/3/reference/datamodel.html#type.__subclasses__) to tests the configs with subclasses. Reviewed By: anana10c Differential Revision: D67652761 fbshipit-source-id: 58d3131052f0f0d01e37c49506761209f069e38a
No description provided.