Skip to content

Build mnnvl_moe_alltoall with logger and stringUtils#2807

Open
tiran wants to merge 1 commit intoflashinfer-ai:mainfrom
tiran:moe_alltoall-missing-symbols
Open

Build mnnvl_moe_alltoall with logger and stringUtils#2807
tiran wants to merge 1 commit intoflashinfer-ai:mainfrom
tiran:moe_alltoall-missing-symbols

Conversation

@tiran
Copy link
Contributor

@tiran tiran commented Mar 17, 2026

📌 Description

The mnnvl_moe_alltoall module uses Logger::getLogger and fmtstr symbols. Add dependencies to logger.cpp and stringUtils.cpp to include the symbols in the shared library.

🔍 Related Issues

#2804

🚀 Pull Request Checklist

Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.

✅ Pre-commit Checks

  • I have installed pre-commit by running pip install pre-commit (or used your preferred method).
  • I have installed the hooks with pre-commit install.
  • [ x I have run the hooks manually with pre-commit run --all-files and fixed any reported issues.

If you are unsure about how to set up pre-commit, see the pre-commit documentation.

🧪 Tests

  • Tests have been added or updated as needed.
  • All tests are passing (unittest, etc.).

Reviewer Notes

I have not tested the build locally, yet.

Summary by CodeRabbit

  • Chores
    • Updated internal compilation configuration to include additional utility source files in the MOE alltoall module build.

The `mnnvl_moe_alltoall` module uses `Logger::getLogger` and `fmtstr`
symbols. Add dependencies to `logger.cpp` and `stringUtils.cpp` to
include the symbols in the shared library.

Fixes: flashinfer-ai#2804
Signed-off-by: Christian Heimes <cheimes@redhat.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d9ff21b7-579d-4143-bae4-7da7fa464489

📥 Commits

Reviewing files that changed from the base of the PR and between abf080a and 73387da.

📒 Files selected for processing (1)
  • flashinfer/jit/comm.py

📝 Walkthrough

Walkthrough

The gen_moe_alltoall_module function in the JIT communication module is updated to include two additional source files (logger.cpp and stringUtils.cpp) in the compiled sources for the MOE alltoall kernel module, expanding dependencies without altering public APIs or include paths.

Changes

Cohort / File(s) Summary
JIT Module Configuration
flashinfer/jit/comm.py
Added two source files (nv_internal/cpp/common/logger.cpp and nv_internal/cpp/common/stringUtils.cpp) to the MOE alltoall module's JIT compilation spec.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

Hop, hop—two files now compile,
Logger and strings join the pail,
MOE's alltoall runs with style,
No change to API's tale. 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding logger and stringUtils dependencies to the mnnvl_moe_alltoall build.
Description check ✅ Passed The description includes all required template sections: a clear problem statement with symbol dependencies, related issue #2804, and a completed checklist with pre-commit verification.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can use your project's `pylint` configuration to improve the quality of Python code reviews.

Add a pylint configuration file to your project to customize how CodeRabbit runs pylint.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a build issue for the mnnvl_moe_alltoall module by incorporating necessary C++ source files, logger.cpp and stringUtils.cpp, into its compilation. This ensures that required symbols are available, allowing the module to link correctly and function as intended.

Highlights

  • Dependency Addition: Added logger.cpp and stringUtils.cpp as dependencies to the mnnvl_moe_alltoall module build process.
  • Symbol Resolution: The added dependencies are intended to resolve missing Logger::getLogger and fmtstr symbols during the build of the shared library.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • flashinfer/jit/comm.py
    • Added logger.cpp to the list of source files for mnnvl_moe_alltoall module compilation.
    • Added stringUtils.cpp to the list of source files for mnnvl_moe_alltoall module compilation.
Activity
  • Pre-commit checks have been completed and passed by the author.
  • Tests have not yet been added or updated, and all tests are not yet passing.
  • The author has not yet tested the build locally.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly adds logger.cpp and stringUtils.cpp as source dependencies to the mnnvl_moe_alltoall JIT module, which should resolve the linking errors for Logger::getLogger and fmtstr symbols. The change is straightforward and necessary. I have one suggestion to improve code maintainability by refactoring the construction of source file paths to reduce repetition.

Comment on lines +104 to +113
jit_env.FLASHINFER_CSRC_DIR
/ "nv_internal"
/ "cpp"
/ "common"
/ "logger.cpp",
jit_env.FLASHINFER_CSRC_DIR
/ "nv_internal"
/ "cpp"
/ "common"
/ "stringUtils.cpp",
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

These new entries add to the repetition of long path prefixes in this source list. To improve readability and maintainability, consider defining a base path for the common C++ sources and reusing it for all related files.

For example:

    common_cpp_base = jit_env.FLASHINFER_CSRC_DIR / "nv_internal" / "cpp" / "common"
    sources = [
        # ... other sources
        common_cpp_base / "envUtils.cpp",
        common_cpp_base / "tllmException.cpp",
        common_cpp_base / "logger.cpp",
        common_cpp_base / "stringUtils.cpp",
    ]

This would make the list of sources cleaner and easier to manage. Since this refactoring would affect lines outside the current diff, it could be addressed in a follow-up.

@yzh119 yzh119 marked this pull request as ready for review March 17, 2026 20:31
@yzh119 yzh119 added the run-ci label Mar 17, 2026
@yzh119
Copy link
Collaborator

yzh119 commented Mar 17, 2026

/bot run

@flashinfer-bot
Copy link
Collaborator

GitLab MR !420 has been created, and the CI pipeline #46370602 is currently running. I'll report back once the pipeline job completes.

@flashinfer-bot
Copy link
Collaborator

[FAILED] Pipeline #46370602: 1/20 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants