Nit: Can two different FunctionProtos in this list have conflicting imports? (I'm guessing they can't, but the wording here is ambiguous.)

Also, is it useful to be able to control the opset imports on a per-function basis, given this restriction? Would it be more ergonomic for the functions to inherit the imports from the containing ModelProto?

Yes, this originates from the use of FunctionProto defined outside of models (in defining standard ops as functions). We could potentially distinguish between the two usages and restrict some fields to be used only in FunctionProtos outside of models.

ONNX does not allow importing different versions of the same domain so there cannot be conflicting imports...

opset imports were added to FunctionProto to allow functions to easily import opsets other than what the model imports... the restriction is only on the version of the domain being unique within the model...

Does the set of functions in a ModelProto count as an "OperatorSet" for the purposes of this comment? (i.e. Can a model-local function refer to other model-local functions?)

I think we should allow a model-local function to refer to other model-local functions.

Yes one model local function can refer to another model local function...

more and more I think about this I am leaning towards adding domain field to FunctionProto...

How does the model refer to these functions? If they were in an operator set, presumably there would be a domain, but I don't see a domain here.

Should this field be an OperatorSetProto (or some subset of that proto)?

FunctionProto has a "name" field. The model compares this with the op_type field from NodeProto.

Rama and I were discussing whether we want domain qualified names, meaning should we add domain field to the functionproto as well... we can do that if we think just the name field wont be enough...

Two possible options:
(a) We add a domain field to FunctionProto. (We then need to decide whether we want to allow these in-model functions to override out-of-model functions defined in opschema registries.)
(b) Reserve a domain for in-model functions (like 'onnx.ai.local') if we want to avoid the above.

added domain field for FunctionProto

IIUC, the check here means that every function in a domain "foo" must have an entry for "foo" in its opset_import with a particular version, which becomes domain_version here. Is that right?

This logic seems a little tortuous... would it make more sense to have a domain_version attribute on the FunctionProto? Or - perhaps better - the version could be inferred from the opset_import on the containing model (or inferred from the op schema, if the function is not a model-local function)?

At the very least, we need to document this requirement, because it was not obvious to me from reading the protobuf spec.

[EDIT: I see code in schema.cc that appears to add this information if it's missing. Is it still required for model-local functions?]

Right every function in a domain "Foo" must have an entry for "foo" in its opset_import with a particular version.

For function ops - If this info is missing then it is inferred ( from op schema ) and implicitly added during schema finalization.

For model local functions - this info is required to be present. We can infer the version for the function from model opset imports but I wanted to reduce the differences in construction or interpretation of FunctionProto for FunctionOps vs model local functions so I decided not to make an exception for model local functions in this case.

Let me know your thoughts...

There seem to be some corner cases where it is worth documenting what the standard requires. (a) If we have a model-local function "bar" in a domain "foo", is the model/graph required to have an import for "foo" (even though the model does not contain any calls to anything other than "foo")? (b) If so, if the FunctionProto for "bar" also includes an import for "foo", is it allowed to be different from what the graph specifies? The opset-compatibility discussion does not clarify this special-case. It doesn't seem important to allow them to be different in this case. (c) If we don't allow to be different, forcing the model to include the same information in two places seems less desirable. So, inferring it from the model's imports may be reasonable.

In general, it seems that model-local functions should not need to use opset-imports and should be able to get away with the model's own opset-imports (for the ops called from the function body). So, doing the same for the function's own domain (look it up from the model's imports) seems reasonable. It is also important to clarify which of the three options we go with (i) required, (ii) allowed but not required, or (iii) not allowed. In functions outside models, we do need to allow opset imports.