Skip to content

Fix AQT Test Suite & Fix module import #737

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

Merged
merged 11 commits into from
Nov 4, 2024

Conversation

vishesh9131
Copy link
Contributor

@vishesh9131 vishesh9131 commented Oct 6, 2024

Pull Request: Fix Incorrect Module Import in layers.py

Now axlearn supports python 3.10 thus the PR Is valid now.

Summary

This pull request addresses an issue with an incorrect module import in the axlearn/common/quantized_dot_general/layers.py file. The import and usage of the Context class from the aqt_dot_general module have been corrected to use the appropriate Context class from the aqt_utils module.

Changes

1. Import Correction:

Now axlearn supports python 3.10 thus the PR Is valid now.

  • Changed the import from aqt.jax.v2.aqt_dot_general to aqt.jax.v2.utils and aliased it as aqt_utils.

2. Context Class Usage:

  • Updated the usage of the Context class to use aqt_utils.Context instead of aqt_dot_general.Context.

New Changes (14Oct)

Explained:

  1. Removed the import of NoNumerics from aqt.jax.v2.config.
  2. Removed the import of IntNumerics from aqt.jax.v2.int_numerics.
  3. Added imports for int_numerics and no_numerics from aqt.jax.v2.numerics.

Testing

  • Verified that the changes resolve any import-related issues and the code runs successfully without any errors.

Additional Information

Code Changes

1. Importing aqt_utils

from aqt.jax.v2 import utils as aqt_utils

2. Taken Reference from the official README of aqt

context: aqt_dot_general.Context = aqt_dot_general.Context(key=None, train_step=None),

to

context: aqt_utils.Context = aqt_utils.Context(key=None, train_step=None),

Testing

  • Verified that the changes resolve the AttributeError and the code runs successfully without any issues.

my mail : [email protected]

These enhancements provide additional flexibility and options for implementing and experimenting with different recurrence methods in the Mamba and Jamba models, potentially improving performance and accuracy for various tasks.
- fixed functions redundant definitions
- fixed Incorrect Module Import in layers.py
@jiarui-lu2
Copy link
Contributor

Thanks! @vishesh9131 you'll also need to bump aqt version in https://github.com/apple/axlearn/blob/2782df73b20eb55f218f501e5df4d0fe19f72227/pyproject.toml#L43C5-L43C18. After that I can trigger the PR checks.

@vishesh9131
Copy link
Contributor Author

Hi @jiarui-lu2 ,
Yes, I've updated the aqtp version in the pyproject.toml file as well. You can go ahead and trigger the PR checks.
Thanks!

@vishesh9131
Copy link
Contributor Author

@jiarui-lu2 sorry for typo error...

@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee

Could you please take a look at my PR when you get a chance? Thanks!

@vishesh9131
Copy link
Contributor Author

@jiarui-lu2
pls can you help me why it is failed again...

@jiarui-lu2
Copy link
Contributor

As the CI states, looks like there are tests that requires fixing as well

FAILED: /home/circleci/project/.pytype/pyi/axlearn/common/quantized_dot_general/utils_test.pyi 
/home/circleci/.pyenv/versions/3.10.15/bin/python3.10 -m pytype.single --imports_info /home/circleci/project/.pytype/imports/axlearn.common.quantized_dot_general.utils_test.imports --module-name axlearn.common.quantized_dot_general.utils_test --platform linux -V 3.10 -o /home/circleci/project/.pytype/pyi/axlearn/common/quantized_dot_general/utils_test.pyi --analyze-annotated --nofail --quick /home/circleci/project/axlearn/common/quantized_dot_general/utils_test.py
File "/home/circleci/project/axlearn/common/quantized_dot_general/utils_test.py", line 8, in <module>: Can't find module 'aqt.jax.v2.int_numerics'. [import-error]

@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee
I have revised my PR…
Could you please take a look at my PR when you get a chance? Thanks!

- Check if cfg.fwd.lhs is of the expected type or has the expected attribute.
@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee
I have fixed my PR again…
I am not able to debug why tests are failing,

Could you please take a look at my PR when you get a chance? Thanks!

@markblee
Copy link
Contributor

Hi @vishesh9131, you can view your CI logs in circleci, e.g.: https://app.circleci.com/pipelines/github/apple/axlearn/2776/workflows/84ee8526-8e05-4de1-ae48-8354f607af36/jobs/5975

@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee

fixed that silly assertion ,
Could you please take a look at my PR when you get a chance? Thanks!

@jiarui-lu2
Copy link
Contributor

I've started the CI again

- replaced with latest attr names used by aqt team.
- added some important assertions.
- now its passing all tests.
@vishesh9131
Copy link
Contributor Author

Dear @jiarui-lu2
@markblee
@ruomingp

Update and Fix AQT Test Suite

This pull request revision addresses and resolves issues in the AQT test suite by updating the test cases to align with the current structure and attributes of the AQT configuration classes. After a thorough review of the AQT codebase, the following changes have been implemented:

Changes Made

  1. Updated Test Assertions:

    • Revised the test cases for lhs_activation_aqt_config and rhs_activation_aqt_config to correctly reflect the attributes of the DotGeneral and Tensor classes.
    • Ensured that the tests check for the correct types and values of attributes such as use_fwd_quant, dequant_mode, calibration_mode, and accumulator_dtype.
  2. Improved Test Coverage:

    • Added assertions to verify the presence and correctness of stochastic rounding functions where applicable.
    • Ensured that the numerics configurations are correctly validated for both forward and backward paths.

Testing

  • After iterations of Failures Now All tests have been executed successfully, confirming that the updates do not introduce any regressions.
  • The test suite now provides comprehensive coverage of the AQT configurations, ensuring robust validation.

Cheers!

@vishesh9131
Copy link
Contributor Author

vishesh9131 commented Oct 29, 2024

@markblee
@ruomingp
Now its passing all tests.
Favour me to pass it to CI.
Thanks for understanding.

@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee
I have revised my PR…
Could you please take a look at my PR when you get a chance? Thanks!

@jiarui-lu2
Copy link
Contributor

Retriggered CI

@vishesh9131 vishesh9131 changed the title Fix module import 2 Fix AQT Test Suite & Fix module import Nov 1, 2024
@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee
I hope this message finds you well. The PR has successfully passed all CI checks. At your convenience, could you please review it? Your approval and merge would be greatly appreciated.

Copy link
Contributor

@ruomingp ruomingp left a comment

Choose a reason for hiding this comment

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

Will defer to @jiarui-lu2 for approval.

@vishesh9131
Copy link
Contributor Author

@jiarui-lu2
Thanks for the approval.

@vishesh9131
Copy link
Contributor Author

Dear @ruomingp,

Thank you very much for your approval. May I kindly ask if we could proceed with starting the merge process now?

@vishesh9131
Copy link
Contributor Author

@ruomingp
@markblee

May I kindly ask if we could proceed with starting the merge process now?

@ruomingp ruomingp added this pull request to the merge queue Nov 4, 2024
@ruomingp
Copy link
Contributor

ruomingp commented Nov 4, 2024

Yes, please merge. Thanks!

Merged via the queue into apple:main with commit 1fdcc8c Nov 4, 2024
3 checks passed
@vishesh9131
Copy link
Contributor Author

Dear @jiarui-lu2, @markblee, and @ruomingp,

I wanted to extend my heartfelt thanks for your guidance and support throughout the AQT test suite update process. Your insights and feedback were invaluable in helping me revise and align the test cases with the current structure of the AQT configuration classes. The collaboration made a huge difference, and I’m thrilled to share that we successfully merged the changes into apple/axlearn!

This experience has been incredibly fulfilling, and I’m eager to continue contributing to axlearn. Although I’m not that well-known yet, I’d love to stay connected with you all—please feel free to connect with me on LinkedIn! 😊

Linkedin : LetsConnect

Thanks again, and looking forward to more collaborations!

Cheers,
Vishesh

jiarui-lu2 added a commit to jiarui-lu2/axlearn that referenced this pull request Nov 4, 2024
github-merge-queue bot pushed a commit that referenced this pull request Nov 4, 2024
qdavid1 pushed a commit to qdavid1/axlearn that referenced this pull request Dec 11, 2024
* ssm_enhancement

These enhancements provide additional flexibility and options for implementing and experimenting with different recurrence methods in the Mamba and Jamba models, potentially improving performance and accuracy for various tasks.

* some fixes

- fixed functions redundant definitions
- fixed Incorrect Module Import in layers.py

* restart

* Update layers.py

* Update pyproject.toml

* Update pyproject.toml

* fixed jax.v2 imports

* cfg.fwd.lhs type expected fix

- Check if cfg.fwd.lhs is of the expected type or has the expected attribute.

* fixed some assertions

* changes:

- replaced with latest attr names used by aqt team.
- added some important assertions.
- now its passing all tests.
qdavid1 pushed a commit to qdavid1/axlearn that referenced this pull request Dec 11, 2024
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.

4 participants