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

Allow flags to be set with greater flexibility #17659

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

daveliddell
Copy link
Contributor

@daveliddell daveliddell commented Jun 13, 2024

Changes to the python binding to allow iree.runtime.flags.parse_flags to take effect at times other than before the first time a driver is created. Also includes fixes for bugs exposed during the development of this feature.

  • Added "internal" API functions create_hal_driver() and clear_hal_driver_cache() to create a driver object independent of the cache, and to clear the cache, respectively
  • Added HalDriver class implementation functions for the above new API functions. Refactored class to share as much common code as possible.
  • Factored out driver URI processing into its own nested class for easier handling of URI components
  • Fixed dangling pointer bug. In the C layer flags are being kept by reference as string views, requiring the caller to keep the original flag strings (argc, argv) around for as long as the flags are being used. However, the python binding was using a local variable for those strings, letting them go out of scope and causing garbage values later on. The fix is to move the strings to a file scope variable. Flag handling does not appear to be getting used in a multi-threaded environment, as other aspects of flag handling use static variables with no mutex guarding that I could find.
  • Fixed runtime assert in Windows debug build for the improper use of std::vector<>::front() on an empty vector. The code never used the value of front(), as it was guarded by a check for the vector's size, but the assert prevents the debug build from running.

Signed-off-by: Dave Liddell <dave.liddell@amd.com>
Signed-off-by: daveliddell <dave.liddell@amd.com>
@daveliddell daveliddell marked this pull request as ready for review June 13, 2024 02:51
@ScottTodd ScottTodd added the bindings/python Python wrapping IREE's C API label Jun 13, 2024
@ScottTodd ScottTodd requested a review from benvanik June 13, 2024 16:48
@daveliddell daveliddell marked this pull request as draft June 13, 2024 18:15
@daveliddell
Copy link
Contributor Author

Please hold off reviewing; will rework client of this API not to use the driver cache, so will git rid of unnecessary new features

@daveliddell
Copy link
Contributor Author

Snippet of ugly but functional use of this new feature:

class vmfbRunner:
    def __init__(self, device, vmfb_path, external_weight_path=None, extra_plugin=None):
        flags = []
        clean_driver = False
        if extra_plugin:
            ireert.flags.parse_flags(f"--executable_plugin={extra_plugin}")
            clean_driver = True
        haldriver = ireert.get_driver(device, clean_driver)

runtime/bindings/python/hal.cc Outdated Show resolved Hide resolved
runtime/bindings/python/hal.cc Show resolved Hide resolved
runtime/bindings/python/hal.cc Outdated Show resolved Hide resolved
runtime/bindings/python/hal.cc Outdated Show resolved Hide resolved
runtime/bindings/python/initialize_module.cc Outdated Show resolved Hide resolved
runtime/bindings/python/iree/runtime/system_setup.py Outdated Show resolved Hide resolved
@daveliddell
Copy link
Contributor Author

General note: in an offline discussion, we decided to keep the cache, so this PR is still a go. Making it ready-for-review again

@daveliddell daveliddell marked this pull request as ready for review June 13, 2024 21:53
Signed-off-by: Dave Liddell <dave.liddell@amd.com>
@daveliddell
Copy link
Contributor Author

New sample usage:

class vmfbRunner:
    def __init__(self, device, vmfb_path, external_weight_path=None, extra_plugin=None):
        flags = []

        # If an extra plugin is requested, add a global flag to load the plugin
        # and create the driver using the non-caching creation function, as
        # the caching creation function may ignore the flag.
        if extra_plugin:
            ireert.flags.parse_flags(f"--executable_plugin={extra_plugin}")
            haldriver = create_hal_driver(device)

        # No plugin requested: create the driver with the caching create
        # function.
        else:
            haldriver = ireert.get_driver(device)

Copy link
Collaborator

@stellaraccident stellaraccident left a comment

Choose a reason for hiding this comment

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

Approving but please make the requested changes to the flag vector before landing as it has a subtle bug how it is.

runtime/bindings/python/initialize_module.cc Outdated Show resolved Hide resolved
runtime/bindings/python/initialize_module.cc Outdated Show resolved Hide resolved
Signed-off-by: Dave Liddell <dave.liddell@amd.com>
@ScottTodd
Copy link
Collaborator

CI failure is preexisting and unrelated to this change, merging through it.
https://github.com/iree-org/iree/actions/runs/9512652449/job/26221447494?pr=17659

[ RUN      ] NCCLDynamicSymbolsTest.CreateFromSystemLoader
iree/runtime/src/iree/hal/drivers/hip/dynamic_symbols_test.cc:92: Failure
Expected equality of these values:
  21803
  nccl_version
    Which is: 21806

@ScottTodd ScottTodd merged commit c5d4b96 into iree-org:main Jun 14, 2024
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bindings/python Python wrapping IREE's C API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants