-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Generate the ONNX and ONNX-ML operator doc in one run #3467
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments. Most are nit picks... 1 regarding type info needs to be fixed to turn CIs green
We need to setup env var `ONNX_ML` and run `gen_doc.py` twice to update the operator docs if both _ONNX_ and _ONNX-ML_ doc need to be updated. With this change, `python onnx/defs/gen_doc.py` generates _ONNX_ and _ONNX-ML_ doc in one run. Signed-off-by: Zhenhua Wang (王振华) <zhenhua.wzh@icloud.com>
Signed-off-by: Zhenhua Wang (王振华) <zhenhua.wzh@icloud.com>
d3fe877
to
706bed7
Compare
Hi @askhade thank you for the quick review. I have fixed your concerns, please help to check. |
Signed-off-by: Zhenhua Wang (王振华) <zhenhua.wzh@icloud.com>
Hmmm... Cannot reproduce the error with python 3.8. When trying to repro with the steps of the pipeline |
Close since run twice for ONNX and ONNX-ML should be by design. Thanks for the review! |
@jackwish : I think you missunderstood the comment. Your PR is a good enhancement for the current scenario it just needs a little change. When ONNX_ML is set to 0 when building onnx we do not build ml ops and therefore if you run the gen doc script it will create an empty operators-ml.md file. This is what needs to be fixed. In CIs we do set ONNX_ML to 0 and after running gen_doc.py it produces as empty operators-ml.md and therefore CIs are failing. I hope you will reopen it and make this fix. |
### Description <!-- - Describe your changes. --> Generate both onnx and onnx-ml operator docs when ONNX_ML=1 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Closes #5377. Similar to #3467. Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com>
### Description <!-- - Describe your changes. --> Generate both onnx and onnx-ml operator docs when ONNX_ML=1 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Closes onnx#5377. Similar to onnx#3467. Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com> Signed-off-by: Megan Hampton <hamptonm@us.ibm.com>
### Description <!-- - Describe your changes. --> Generate both onnx and onnx-ml operator docs when ONNX_ML=1 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Closes onnx#5377. Similar to onnx#3467. Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
### Description <!-- - Describe your changes. --> Generate both onnx and onnx-ml operator docs when ONNX_ML=1 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Closes onnx#5377. Similar to onnx#3467. Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
### Description <!-- - Describe your changes. --> Generate both onnx and onnx-ml operator docs when ONNX_ML=1 ### Motivation and Context <!-- - Why is this change required? What problem does it solve? --> <!-- - If it fixes an open issue, please link to the issue here. --> Closes onnx#5377. Similar to onnx#3467. Signed-off-by: Chun-Wei Chen <jacky82226@gmail.com> Co-authored-by: Xavier Dupré <xadupre@users.noreply.github.com> Signed-off-by: Aditya Goel <agoel4512@gmail.com>
We need to setup env var
ONNX_ML
and rungen_doc.py
twice to update the operator docs if both ONNX and ONNX-ML doc need to be updated.With this change,
python onnx/defs/gen_doc.py
generates ONNX and ONNX-ML doc in one run.