Skip to content

Is "validate graph resources" backwards? #602

Description

@inexorabletash

Currently, compute() calls validate graph resources with the passed input (or output) maps and the graph's input (or output) descriptors.

I believe the intent is to ensure that all of a graph's input (or output) descriptors are matched to a (name, buffer) pair in the passed input (or output) resources.

The steps are:

To validate graph resources, given MLNamedArrayBufferViews resources and ordered map descriptors, run the following steps:

  1. For each name → resource of resources:
    1. If descriptors[name] does not exist, return false.
    2. If validating buffer with descriptor given resource and descriptors[name] returns false, then return false.
  2. Return true.

This instead seems to be ensuring that each (name, buffer) pair in the passed resources has a corresponding input (or output) descriptor - which is similar but backwards.

i.e.

const builder = new MLGraphBuilder(context);
const input1 = builder.input('input1', desc1);
const input2 = builder.input('input2', desc2);
const output1 = builder.mul(input1, input2);
const graph = await builder.build({'output1': output1});
// graph has two inputs (input1, input2) and one output (output1)

// Expected usage
await context.compute(graph, {input1: inbuf1, input2: inbuf2}, {output1: outbuf1});

// Should fail, because input2 is missing - but it *doesn't*.
await context.compute(graph, {input1: inbuf1}, {output1: outbuf1});

// Maybe fail, because output2 is passed but unused. Currently it *does* fail. 
await context.compute(graph, {input1: inbuf1, input2: inbuf2}, {output1: outbuf1, output2: outbuf2});

It seems like we've got two options:

  • Invert the test - iterate over the descriptors, and verify that there are matching resources
  • Make the test reflexive - the set of expected and passed resources must exactly match

Activity

  1. inexorabletash commented on Mar 26, 2024

    @inexorabletash
    ContributorAuthor

    It looks like Chromium does a reflexive test - I'll put up a PR for that.

  2. inexorabletash commented on Apr 4, 2024

    @inexorabletash
    ContributorAuthor

    Note that in the PR #622 we ended up with a different approach, not a reflexive test. So please take a look over there.

  3. added a commit that references this issue on Apr 16, 2024
    fd8a8d0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions