Skip to content

[path_finder_dev] path_finder Search Priority v2 - #604

Merged
rwgk merged 23 commits into
NVIDIA:path_finder_devfrom
rwgk:path_finder_search_priority_v2
May 6, 2025
Merged

[path_finder_dev] path_finder Search Priority v2#604
rwgk merged 23 commits into
NVIDIA:path_finder_devfrom
rwgk:path_finder_search_priority_v2

Conversation

@rwgk

@rwgkrwgk commented May 4, 2025

Copy link
Copy Markdown
Contributor

Continuation of #578

Completely replace cuda_paths.py to achieve the desired Search Priority (see updated README.md).

Further testing is under #610

@copy-pr-bot

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgkrwgk self-assigned this May 4, 2025
@rwgk
rwgkforce-pushed the path_finder_search_priority_v2 branch from 6a49659 to 8ddb806CompareMay 5, 2025 03:33
@rwgk
rwgkforce-pushed the path_finder_search_priority_v2 branch from 8ddb806 to 2cf3fa2CompareMay 5, 2025 04:46
@rwgk

rwgk commented May 5, 2025

Copy link
Copy Markdown
ContributorAuthor

/ok to test 2b74022

@github-actions

Copy link
Copy Markdown

@rwgk

rwgk commented May 5, 2025

Copy link
Copy Markdown
ContributorAuthor

/ok to test 27db0a7

@rwgk
rwgk marked this pull request as ready for review May 5, 2025 08:41
@copy-pr-bot

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgk
rwgk requested review from kkraus14 and leofangMay 5, 2025 08:42
Comment threadcuda_bindings/cuda/bindings/_path_finder/README.md
Comment threadcuda_bindings/cuda/bindings/_path_finder/find_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_dl_common.py Outdated
@rwgk

rwgk commented May 5, 2025

Copy link
Copy Markdown
ContributorAuthor

/ok to test b1a5e9d

Comment threadcuda_python/docs/source/release/12.9.0-notes.rst
The previous implementation checked result_queue.empty() before calling get(),
which introduces a classic race condition: the queue may become non-empty
immediately after the check, resulting in missed results or misleading errors.
This patch replaces the empty() check with result_queue.get(timeout=1.0),
allowing the parent process to robustly wait for results with a bounded delay.
Also switches from ctx.SimpleQueue() to ctx.Queue() for compatibility with
timeout-based get(), which SimpleQueue does not support on Python ≤3.12.
Note: The race condition was discovered by Gemini 2.5
@rwgk

rwgk commented May 5, 2025

Copy link
Copy Markdown
ContributorAuthor

Tracking the development history of run_python_code_safely.py up to commit 9b474bc:

  1. Perplexity_run_python_code_safely_original_version.pdf
  2. ChatGPT Deep research followed by feeding back Gemini 2.5 findings
  3. Claude_3.7_Sonnet_2025-05-05+131336.pdf — This didn't lead to any changes.
  4. Gemini_2.5_2025-05-05+132203.pdf — Discovered a "classical race condition"

Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/README.md Outdated
@leofang

Copy link
Copy Markdown
Member

It is pretty awesome that the search priority update also allows us to drop the old Numba code entirely!

@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.

Looking good! Several previous discussions on exec and subprocess/multiprocessing are now moot by removing these unused code entirely.

Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_nvidia_dynamic_library.py Outdated
Comment threadcuda_bindings/cuda/bindings/_path_finder/load_dl_common.py Outdated
…od. Move run_python_code_safely.py to test/ directory.
@rwgkrwgk mentioned this pull request May 6, 2025
@rwgk

rwgk commented May 6, 2025

Copy link
Copy Markdown
ContributorAuthor

This PR has base branch path_finder_dev. — I'm keeping it around because it's easiest to look at for a final review.

PR #613 has base branch main, but against the same branch.

Interesting/surprising: I triggered the CI under #613, but it shows up here, too.

@rwgk
rwgk merged commit 16a3a57 into NVIDIA:path_finder_devMay 6, 2025
@rwgk
rwgk deleted the path_finder_search_priority_v2 branch May 6, 2025 16:53
@rwgk
rwgk restored the path_finder_search_priority_v2 branch May 6, 2025 16:55
Comment threadcuda_bindings/cuda/bindings/_path_finder/README.md
Comment threadcuda_bindings/tests/run_python_code_safely.py
Comment threadcuda_bindings/tests/run_python_code_safely.py
rwgk added a commit to rwgk/cuda-python that referenced this pull request May 7, 2025
@rwgkrwgk changed the title WIP: path_finder Search Priority v2[path_finder_dev] path_finder Search Priority v2May 8, 2025
rwgk added a commit that referenced this pull request May 9, 2025
* Use win32api.GetModuleFileName() in abs_path_for_dynamic_library(). With this, load_dl_windows.py consistently uses win32api. ctypes is no longer needed, which eliminates the potential for confusion due to different types of handles.
* Address review comment #604 (comment) by at-kkraus14
* Rename function run_python_code_safely() → run_in_spawed_child_process()
* Change run_in_spawned_child_process() to accept a callable function instead of a string with Python code.
* ChatGPT suggestions
* Add rethrow as suggested by ChatGPT
* Better names: Worker → ChildProcessWrapper, func → target
* ChatGPT suggestions
* Add minimal test_spawned_process_runner.py as generated by ChatGPT
Sign up for freeto 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.

3 participants

@rwgk@leofang@kkraus14