Skip to content

Conversation

@rdspring1 rdspring1 added Python API Issues related to the Python API Direct Bindings Python extension with direct mapping to NvFuser CPP objects. labels Jul 28, 2025
Copy link

github-actions bot commented Jul 28, 2025

Review updated until commit 3b46dd8

Description

  • Added Iota operation to direct Python bindings

  • Implemented Python translator handling for IotaOp

  • Added test cases for Iota operation functionality

  • Updated opinfo metadata to support direct bindings


Changes walkthrough 📝

Relevant files
Enhancement
ops.cpp
Add Iota op binding and factory registration                         

python/python_direct/ops.cpp

  • Added iota operation binding with parameters (length, start, step,
    dtype)
  • Added bindTensorFactoryOps function to register new ops
  • Updated bindOperations to include tensor factory ops
  • +33/-1   
    python_translate.cpp
    Implement IotaOp translation to Python                                     

    python/python_direct/python_translate.cpp

  • Added handle implementation for IotaOp translation
  • Set up argument dispatching and default values
  • Generated keyword argument operation for Python frontend
  • +23/-0   
    Tests
    test_python_frontend.py
    Add test cases for Iota operation                                               

    tests/python/direct/test_python_frontend.py

  • Added test_iota function with multiple input configurations
  • Verified output correctness against eager mode execution
  • Tested different data types and parameter combinations
  • +23/-0   
    opinfos.py
    Update Iota opinfo metadata for direct bindings                   

    tests/python/opinfo/opinfos.py

  • Added supports_direct_bindings=True to Iota opinfo
  • Updated symbolic parameter list for Iota operation
  • +1/-0     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Incorrect Step Default

    The docstring indicates that the step parameter defaults to zero when None, which would produce incorrect or unintended tensor values as a zero step would not increment. This may conflict with the expected behavior of the iota operation.

    step : Val, optional
        The step of the tensor. When the default is None, step is set to zero.
    Potential Null Pointer Handling

    The code dispatches iop->start() and iop->step() even when they are optional parameters. If these values are null, this could lead to undefined behavior unless the dispatch function explicitly handles null pointers.

    dispatch(iop->start());
    dispatch(iop->step());
    Incomplete Test Coverage

    The added test cases explicitly provide all parameters (length, start, step, dtype). Missing tests for default values (e.g., omitting start or step) may leave edge cases unvalidated.

    inputs = [
        (2, 0, 2, DataType.Int),
        (3, 100, 1, DataType.Int32),
    ]
    
    def fusion_func(fd: FusionDefinition):
        for input in inputs:
            c0 = fd.define_scalar(input[0])
            c1 = None if input[1] is None else fd.define_scalar(input[1])
            c2 = None if input[2] is None else fd.define_scalar(input[2])
            dt = input[3]
            t3 = fd.ops.iota(c0, c1, c2, dt)
            fd.add_output(t3)

    @rdspring1
    Copy link
    Collaborator Author

    !build

    @rdspring1 rdspring1 requested a review from jjsjann123 July 29, 2025 16:37
    rdspring1 added a commit that referenced this pull request Jul 30, 2025
    rdspring1 added a commit that referenced this pull request Jul 30, 2025
    rdspring1 added a commit that referenced this pull request Jul 31, 2025
    Base automatically changed from direct_tp14 to main July 31, 2025 01:47
    @jjsjann123
    Copy link
    Collaborator

    Can we resolve the conflict first? I think this PR is picking up things from #4876

    @rdspring1
    Copy link
    Collaborator Author

    @jjsjann123 I rebased this PR.

    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a comment

    Choose a reason for hiding this comment

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

    one nitpick about the python signature of iota.

    @rdspring1
    Copy link
    Collaborator Author

    !build

    @rdspring1 rdspring1 merged commit 3db1b12 into main Aug 3, 2025
    17 checks passed
    @rdspring1 rdspring1 deleted the direct_tp15 branch August 3, 2025 19:52
    rdspring1 added a commit that referenced this pull request Aug 3, 2025
    rdspring1 added a commit that referenced this pull request Aug 3, 2025
    rdspring1 added a commit that referenced this pull request Aug 3, 2025
    rdspring1 added a commit that referenced this pull request Aug 4, 2025
    rdspring1 added a commit that referenced this pull request Aug 4, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Direct Bindings Python extension with direct mapping to NvFuser CPP objects. Python API Issues related to the Python API

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants