Skip to content

Fix linker file path support, add ObjectCode constructor tests for ltoir inputs, and expose code_type - #890

Merged
leofang merged 15 commits into
mainfrom
copilot/fix-663
Oct 7, 2025
Merged

Fix linker file path support, add ObjectCode constructor tests for ltoir inputs, and expose code_type#890
leofang merged 15 commits into
mainfrom
copilot/fix-663

Conversation

CopilotAI commented Aug 22, 2025

Copy link
Copy Markdown
Contributor
  • Fix linker file path support for ObjectCode constructors
  • Add comprehensive test coverage for ObjectCode constructor methods
  • Flatten nested if statements for better readability
  • Remove redundant tests and improve test organization
  • Implement NVCC-based testing for fatbin and object/library tests
  • Add missing test_object_code_load_ptx_from_file and group tests properly
  • Remove self-explanatory comments and clarify conditional logic in linker

Bug Fix Complete: Fixed TypeError: Expected type bytes, but got str when using ObjectCode instances created from file paths with the Linker class.

Test Coverage Complete: Added comprehensive tests for all ObjectCode constructor methods (from_ltoir, from_fatbin, from_object, from_library) with both file-based and in-memory variants, plus integration tests with the Linker class.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@copy-pr-bot

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

CopilotAIand others added 2 commits August 22, 2025 02:08
…object, and from_library constructors
Co-authored-by: leofang <5534781+leofang@users.noreply.github.com>
…value validation
Co-authored-by: leofang <5534781+leofang@users.noreply.github.com>
CopilotAI changed the title [WIP] [FEA]: Tests for ltoir, fatbin, object, and library constructorsAdd comprehensive tests for ObjectCode constructor methodsAug 22, 2025
CopilotAI requested a review from leofangAugust 22, 2025 02:11

@leofangleofang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot I think you missed my comment #663 (comment), in particular Step 1 there. We need to make sure files on disk can be also consumed by the linker.

@github-project-automationgithub-project-automationBot moved this from Todo to In Progress in CCCLAug 24, 2025
Co-authored-by: leofang <5534781+leofang@users.noreply.github.com>
CopilotAI changed the title Add comprehensive tests for ObjectCode constructor methodsFix linker file path support and add comprehensive ObjectCode constructor testsAug 24, 2025
CopilotAI requested a review from leofangAugust 24, 2025 05:50

@leofangleofang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot

Comment threadcuda_core/cuda/core/experimental/_linker.py Outdated
Co-authored-by: leofang <5534781+leofang@users.noreply.github.com>

CopilotAI commented Aug 25, 2025

Copy link
Copy Markdown
ContributorAuthor

@copilot

Step 1 from your original comment is already implemented in the current code. The linker now properly handles both file paths (str) and in-memory data (bytes) for ObjectCode instances:

  • File paths use nvjitlink.add_file() or cuLinkAddFile()
  • In-memory bytes use add_data() methods
  • Added comprehensive test coverage including integration tests

The bug where the linker crashed with "TypeError: Expected type bytes, but got str" when using file-based ObjectCode has been resolved.

CopilotAI requested a review from leofangAugust 25, 2025 00:28

@leofangleofang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot

btw I noticed that there's still one test missing: test_object_code_load_ptx_from_file. Please add it, and make sure it's placed close to test_object_code_load_ptx. Please also group test_object_code_load_cubin and test_object_code_load_cubin_from_file together for better clarity.

Comment threadcuda_core/tests/test_module.py Outdated
Comment threadcuda_core/tests/test_module.py Outdated
Comment threadcuda_core/tests/test_module.py Outdated
Comment threadcuda_core/tests/test_module.py Outdated
Comment threadcuda_core/tests/test_module.py Outdated
…x fixture naming
Co-authored-by: leofang <5534781+leofang@users.noreply.github.com>

CopilotAI commented Aug 25, 2025

Copy link
Copy Markdown
ContributorAuthor

@copilot

btw I noticed that there's still one test missing: test_object_code_load_ptx_from_file. Please add it, and make sure it's placed close to test_object_code_load_ptx. Please also group ...

Added the missing test_object_code_load_ptx_from_file test and grouped the CUBIN and PTX tests together properly. The PTX tests are now adjacent to each other, and CUBIN tests are grouped. Commit: 5609cb1

CopilotAI requested a review from leofangAugust 25, 2025 00:58
@leofang

Copy link
Copy Markdown
Member

pre-commit.ci fix

@leofang

Copy link
Copy Markdown
Member

pre-commit.ci autofix

@leofangleofang closed this Sep 17, 2025
@github-project-automationgithub-project-automationBot moved this from In Progress to Done in CCCLSep 17, 2025
@leofangleofang reopened this Sep 17, 2025
@github-project-automationgithub-project-automationBot moved this from Done to Needs Triage in CCCLSep 17, 2025
@leofangleofang linked an issue Sep 25, 2025 that may be closed by this pull request
1 task
@leofangleofang changed the title Fix linker file path support and add comprehensive ObjectCode constructor testsFix linker file path support, add ObjectCode constructor tests for LTO IR inputs, and expose code_typeOct 6, 2025
@leofang

Copy link
Copy Markdown
Member

To make it easier to review, I've removed the ObjectCode constructor tests for the object, library, and fatbin inputs added by the Copilot. We can revisit this later.

@leofang
leofang marked this pull request as ready for review October 6, 2025 01:34
@leofang

Copy link
Copy Markdown
Member

/ok to test 727ead5

@leofang

Copy link
Copy Markdown
Member

@brandon-b-miller could you review? 🙂

@leofangleofang changed the title Fix linker file path support, add ObjectCode constructor tests for LTO IR inputs, and expose code_typeFix linker file path support, add ObjectCode constructor tests for ltoir inputs, and expose code_typeOct 6, 2025
@leofang

Copy link
Copy Markdown
Member

ping @benhg too

@github-actions

This comment has been minimized.

Comment threadcuda_core/cuda/core/experimental/_linker.py
@github-project-automationgithub-project-automationBot moved this from Needs Triage to In Review in CCCLOct 7, 2025
@leofang
leofang merged commit 9bb7531 into mainOct 7, 2025
71 checks passed
@leofang
leofang deleted the copilot/fix-663 branch October 7, 2025 19:33
@github-project-automationgithub-project-automationBot moved this from In Review to Done in CCCLOct 7, 2025
@github-actions

Copy link
Copy Markdown
Doc Preview CI
Preview removed because the pull request was closed or merged.

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

Labels

bugSomething isn't workingcuda.coreEverything related to the cuda.core moduleP0High priority - Must do!testImprovements or additions to tests

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[FEA]: Tests for ltoir, fatbin, object, and library constructors

3 participants

@leofang@kkraus14