Skip to content

ONNXML.cs needs to be generated at BUILD #2291

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

Closed
codemzs opened this issue Jan 28, 2019 · 2 comments
Closed

ONNXML.cs needs to be generated at BUILD #2291

codemzs opened this issue Jan 28, 2019 · 2 comments
Labels
Build Build related issue

Comments

@codemzs
Copy link
Member

codemzs commented Jan 28, 2019

Currently ONNXML.cs is generated manually using protobuf definition. It is also included as part of code coverage which should not happen and PR #2290 will remove it. We need to make it such that it is clear that file is not authored by ML.NET team and is coming from an external source.

CC: @wschin , @shauheen , @TomFinley

@codemzs codemzs added the Build Build related issue label Jan 28, 2019
@eerhardt
Copy link
Member

I don't agree that this file needs to be generated during the build. Using the same code generation tool and the same version of the protobuf definition, the same C# file will be generated. There is no reason to do that generation on every build.

Another reason to not take this approach is Source Link debugging. If someone was trying to debug into this code, it wouldn't be possible if we didn't have it checked into our source control.

We need to make it such that it is clear that file is not authored by ML.NET team and is coming from an external source.

This is usually done by the following header in the .cs file:

// <auto-generated>
// Generated by the protocol buffer compiler. DO NOT EDIT!
// source: onnx-ml.proto3
// </auto-generated>

@harishsk
Copy link
Contributor

I agree with @eerhardt. The format that ML.NET requires this file to be in is different from the generated version. We can choose to wrap the generated code, but that would be a lot of needless abstraction.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Build Build related issue
Projects
None yet
Development

No branches or pull requests

3 participants