onnx / tutorials

Tutorials for creating and using ONNX models
Apache License 2.0
3.34k stars 626 forks source link

Add CI to validate URLs and enhance Python code quality #263

Closed jcwchen closed 2 years ago

jcwchen commented 2 years ago

Description

Motivation and Context

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging b2d8f0061feda0ae799b8c5da8919fc88e2ddade into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging d3cd17bd956686c43e006c820e1cd6c790af4ceb into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging 2c42d5d0c9cddbbd95f8937c847dffe45ade634a into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging 0775a078af899b1ad7584a687fb9d74218059d4c into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging 33cc7ac0615036cadf44696a79469c894261fd7a into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging 94b850ca98cbed4d7a462fe618284006914977ff into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 2 years ago

This pull request fixes 4 alerts when merging 744907e38f01a84523108cce02b37c3409539fee into ae0202ea5431f67ecfac03afc9987d67581f2809 - view on LGTM.com

fixed alerts:

jcwchen commented 2 years ago

Would it make sense to also activate the checks as part of the CIs for a new PR?

Good question. Currently the URL check in CI only validates modifications (use git diff to get those modified files). I think it should be sufficient because other URL validation which is not modified by that PR should not block the PR. Therefore global URL check will only run weekly.

Also, some of the files, e.g. the workflow_scripts url validators, appear not to have a copyright statement. They should have their original copyrights, or the Apache-2 if there were created by the author of this PR.

Thank you for catching this! I have just added Apache-2 for files under workflow_scripts.

AlexandreEichenberger commented 2 years ago

Currently the URL check in CI only validates modifications (use git diff to get those modified files). I think it should be sufficient because other URL validation which is not modified by that PR should not block the PR. Therefore global URL check will only run weekly.

Does this cover the scenario where a valid URL in the doc becomes stale because the destination website modified their urls?

jcwchen commented 2 years ago

Does this cover the scenario where a valid URL in the doc becomes stale because the destination website modified their urls?

Yes, such a case will be covered by the weekly check -- The CI will validate all URLs under onnx/tutorials to prevent stale URL.