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

Move hostir and multidevice exec to PolymorphicValues. #3930

Merged
merged 107 commits into from
Feb 21, 2025

Conversation

csarofeen
Copy link
Collaborator

No description provided.

csarofeen and others added 30 commits February 11, 2025 11:45
…nelArgumentHolder::createKernelArgumentHolder in favor of a KernelArgumentHolder constructor.
…sion_cache_utils and fusion_kernel_runtime still.
…. Need to disambiguate KernelExecutor::Compile next.
Copy link

github-actions bot commented Feb 20, 2025

Review updated until commit 9f2c1d3

Description

  • Replace IValue with PolymorphicValue in executor interfaces.

  • Update runWithInput methods to use PolymorphicValue.

  • Remove redundant runWithPolymorphicValues method.

  • Update tests to use PolymorphicValue instead of IValue.


Changes walkthrough 📝

Relevant files
Enhancement
executor.cpp
Update runWithInput to use PolymorphicValue                           

csrc/host_ir/executor.cpp

  • Update runWithInput to accept PolymorphicValue instead of IValue.
  • Remove runWithPolymorphicValues method.
  • +1/-12   
    executor.cpp
    Update runWithInput to use KernelArgumentHolder                   

    csrc/multidevice/executor.cpp

  • Update runWithInput to accept KernelArgumentHolder instead of IValue.
  • Replace IValue with PolymorphicValue in input processing.
  • +5/-5     
    fusion_kernel_runtime.cpp
    Update input mapping to use PolymorphicValue                         

    csrc/runtime/fusion_kernel_runtime.cpp

    • Replace IValue with PolymorphicValue in input mapping.
    +2/-2     
    executor.h
    Update runWithInput signature                                                       

    csrc/host_ir/executor.h

  • Update runWithInput method signature to use PolymorphicValue.
  • Remove runWithPolymorphicValues method.
  • +1/-3     
    executor.h
    Update runWithInput signature                                                       

    csrc/multidevice/executor.h

  • Update runWithInput method signature to use KernelArgumentHolder.
  • +1/-1     
    Tests
    test_host_irs.cpp
    Update tests to use PolymorphicValue                                         

    tests/cpp/test_host_irs.cpp

    • Replace IValue with PolymorphicValue in test cases.
    +15/-15 
    test_multidevice_overlap.cpp
    Update tests to use PolymorphicValue                                         

    tests/cpp/test_multidevice_overlap.cpp

    • Replace IValue with PolymorphicValue in test cases.
    +4/-4     
    test_multidevice_pipeline.cpp
    Update runWithInput call                                                                 

    tests/cpp/test_multidevice_pipeline.cpp

    • Update runWithInput call to use KernelArgumentHolder.
    +1/-1     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Function Removal

    The function runWithPolymorphicValues has been removed. Ensure that all functionalities previously handled by this function are now covered by runWithInput.

    std::vector<at::Tensor> HostIrEvaluator::runWithInput(
        const std::unordered_map<Val*, PolymorphicValue>& val_to_PValue) {
      // process input values, converting IValue to PolymorphicValue
      for (const auto& [val, pvalue] : val_to_PValue) {
        expr_evaluator_.bind(val, pvalue);
      }
    
    Function Signature Change

    The signature of runWithInput has changed from taking std::unordered_map<Val*, c10::IValue> to const std::unordered_map<Val*, PolymorphicValue>&. Verify that this change does not break any existing code that relies on the old signature.

    std::vector<at::Tensor> runWithInput(
        const std::unordered_map<Val*, PolymorphicValue>& val_to_PValue);
    Argument Conversion

    The argument passed to runWithInput has changed from args.toC10Array() to args. Ensure that KernelArgumentHolder is compatible with the new runWithInput signature.

    outputs = runtime->runWithInput(args);

    @csarofeen
    Copy link
    Collaborator Author

    !test

    @csarofeen
    Copy link
    Collaborator Author

    nvfuser-ci/jit_binary_tests_20_H100_3/3 — Found 1 failures after 32 minutes https://nv/eyZ/143382544
    This is just a tolerance issue in the test. Safe to ignore.

    @naoyam naoyam self-requested a review February 20, 2025 22:53
    @wujingyue
    Copy link
    Collaborator

    cc @nsarka and @samnordmann

    Copy link
    Collaborator

    @naoyam naoyam left a comment

    Choose a reason for hiding this comment

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

    @wujingyue Could you please take a look at this PR? It looks good to me, but I just wanted to double check with you.

    @csarofeen csarofeen changed the base branch from polymorphic_outs_step_3 to main February 21, 2025 01:29
    @csarofeen csarofeen changed the base branch from main to polymorphic_outs_step_3 February 21, 2025 01:29
    Base automatically changed from polymorphic_outs_step_3 to main February 21, 2025 02:59
    @csarofeen
    Copy link
    Collaborator Author

    !test

    @naoyam
    Copy link
    Collaborator

    naoyam commented Feb 21, 2025

    The Thunder failure seems to be the same as #3919, for which I have a fix #3926. Not sure why it shows up in this PR but not in others.

    @csarofeen csarofeen merged commit b10f9eb into main Feb 21, 2025
    49 of 50 checks passed
    @csarofeen csarofeen deleted the polymorphic_outs_step_4 branch February 21, 2025 13:04
    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