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 setting runtime handles (alternative) #1002

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

fs-eire
Copy link
Contributor

@fs-eire fs-eire commented Oct 22, 2024

This PR is an alternative way of #997 to implement allowing users to set runtime handles.

The basic idea is to create a JSON string representing the Config class from the RuntimeSettings, and the Config class will merge them.

Currently this PR is not working because of the following:

  • The existing JSON parser does not support multiple entries in provider_options
    The following JSON will fail to parse.

    {
      "model": {
          "decoder": {
              "session_options": {
                  "provider_options": [
                      {
                          "webgpu": { }
                      },
                      {
                          "dml": {}
                      }
                  ]
              },
          },
       },
    }
    
  • When trying to specify JSON overlay, two "webgpu" items does not merge.
    genai_config.json:

    {
      "model": {
          "decoder": {
              "session_options": {
                  "provider_options": [
                      {
                          "webgpu": { "abc": "123" }
                      }
                  ]
              },
          },
       },
    }
    

    generated overlay config:

    {
      "model": {
        "decoder": {
          "session_options": {
            "provider_options": [
              {
                "webgpu": {
                  "dawnProcTable": "12345678"
                }
              }
            ]
          }
        }
      }
    }
    

    Result:

    image

This PR depends on a code change to the Config class to support the expected parsing behaviors.

@fs-eire
Copy link
Contributor Author

fs-eire commented Oct 22, 2024

After merged to latest main (takes #1003), it should work now. Need test whether it actually works.

Copy link
Contributor

@guschmue guschmue left a comment

Choose a reason for hiding this comment

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

tested, works well.

src/config.cpp Outdated Show resolved Hide resolved
src/runtime_settings.cpp Outdated Show resolved Hide resolved
@fs-eire fs-eire merged commit d58daf0 into main Oct 24, 2024
12 of 13 checks passed
@fs-eire fs-eire deleted the fs-eire/allow-set-handle-alternative branch October 24, 2024 09:27
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.

3 participants