-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
pyproject.toml, pre-commit, ruff, uv and typing issues, fixes #1119 #1120
Conversation
406ec72
to
5c8ad45
Compare
The "build" workflow went through, you can have a look.
optional
|
Thanks for the great PR @Dronakurl ! Since there is 9000+ lines of changes (not only styling but critical code changes as well), GIthub Review UI is crashing 😅 and it might take weeks for me to review it. It would be faster if you could separate this large PR (dependency change, workflow changes, project structure change, typo fixes, code logic changes) into smaller single purpose PRs. For instance if I know the changes in a PR are only styling updates, I can review those lines faster but with current form, I have to go over each line to see if its a logical update or styling update. It's up to you, I can review both ways, please let me know which way you want to move forward (split this PR into smaller PRs or keep it this way). |
Hi, thanks that you take a look!
I am not yet finished with the pr, so it cannot be merged yet, see the
todos.
It might even take a week or so before I get to solve those remaining
stuff.
I will try to break it down where possible. There is a lot of typing stuff
that can be separated.
|
It’s not hard coded, it is a default, because the code was relying on a
default value for the device. Or was what you describe a feature before and
I accidentally broke it?
|
Then you found an existing bug 😅 |
Dependencies are OK, but pytest tests are not running. Only the CLI tests are running 🤔 https://github.com/obss/sahi/actions/runs/13220591930/job/36915618246?pr=1120 |
This needed to change after ultralytics is now the default model
…n for huggingface
e9770de
to
c6c9ad6
Compare
I think, the PR is now ready for a final review. |
@Dronakurl, thanks a lot for all the work you put in! Please add your profile/name to the contributor's list in the readme between Kadir Nar and Burak Maden 🤗 |
It looks big, but there is a lot of typing stuff, unneeded imports, etc, that can easily be seen doesn't change anything.
I added comments at the files that have noteworthy changes like bugs found from type checking.
Best way to start reading is pyproject.toml. Here you can see that ruff is used and the dependencies are defined.
The exisiting ci.yml pretended to run on different OS types, but actually did only run 3 times on ubuntu.
In this first step, I did not try to fix this.
Often I found syntax like
x: int = None
. I replaced it withx: Optional[int] = None
so the type checker in my IDE (I used pyright) found cases where the code did not handle the None case. I fixed some of the issues around that.What still confuses me in
base.py
is this: Is_object_prediction_list_per_image
a list or a list of lists.I figured that it could be both, so I defined a type for that.
With
uv pre-commit install
, you now have ruff pre-commits, that will do the equivalent of isort, black and flake.Please tell me what you think