Skip to content
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

Disable udeps checking till all unused deps are resolved #969

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

antimora
Copy link
Collaborator

Pull Request Template

Changes

udeps is failing the current main branch since there are many false positives are unresolved.

@antimora
Copy link
Collaborator Author

CCing @Luni-4

I noticed that my PR build is failing: #967

I attempted to fix it but there were more. I think we should disable till we whitelist the false positives.

@Luni-4
Copy link
Collaborator

Luni-4 commented Nov 20, 2023

@antimora

Instead of disabling the check, we can mark the false-positivies as we have done here https://github.com/Tracel-AI/burn/blob/e5c6044062050bcee74d1e15369c9763ad7dd736/burn-dataset/Cargo.toml#L55-L56 in the specific crate. I guess the problem is that 7154755#diff-b1776cd66202f5cfa4b0878c00a6cc9fe41d82b86e303b2c7f485c5877b5e998R22 is not used in the code, that's why it is marked as unused dep. If we remove that dep, can the example be built the same?

@antimora
Copy link
Collaborator Author

@nathanielsimard @Luni-4 this is a blocker. It takes too long to resolve all the issues. Lets disable it first and enable in another PR. I am not even sure how it was possible to enable it in the first place if there were unused dependencies in the first place. Pretty much most are false positive.

My personal take is that we should not enforce upon PR check but rather audit the code time to time because the tooling of udep is huge to install on users computer and it finds too many false positive. Having an unused dependency for a short period is not super urgent.

@Luni-4
Copy link
Collaborator

Luni-4 commented Nov 20, 2023

@antimora

All right. Can we move this check into the publish.yml script? In this way, we can remove unused deps from the current CI, but check them before releasing, so we do not forget to catch errors. Once the dependencies checker will be implemented within xtask, it will be simpler to detect dependencies locally. What do you think?

@antimora
Copy link
Collaborator Author

@Luni-4 it is a great idea! I will update the PR.

@antimora
Copy link
Collaborator Author

@Luni-4 it is a great idea! I will update the PR.

I think it might cause a headache if we do this right before the release. Actions support scheduled scan. I think we should schedule it similar to other security scans.

@antimora
Copy link
Collaborator Author

antimora commented Nov 20, 2023

I filed a ticket (#975) to take up and fix enabling in other issue.

@nathanielsimard nathanielsimard merged commit 5845790 into tracel-ai:main Nov 20, 2023
6 checks passed
@nathanielsimard nathanielsimard deleted the disable-udeps-ci-checking branch November 20, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants