Skip to content
This repository has been archived by the owner on Sep 15, 2023. It is now read-only.

add pylint test to kso-utils #166

Merged
merged 10 commits into from
Sep 6, 2023
Merged

add pylint test to kso-utils #166

merged 10 commits into from
Sep 6, 2023

Conversation

Diewertje11
Copy link
Collaborator

No description provided.

add pylint test to kso-utils. This test is run in the container so that
all requirements are installed. Currently, the test is run while only
checking on errors (specified in the .pylintrc), to not get too many
warnings at once. When we are used to this, we can include more
warnings to improve code quality.

Note: currently the container to use is hard coded, but if we then
update the requirements and do not have a pull to the
kso-object-detection yet, we will not have this present here to do the
pylint test. So this need to change in some way: *Or change entire
structure repos *Or just install packages here instead of opening the
container, however, this will have the problem that it cannot see the
requirements from yolov5 and yolov5_tracker which are needed.
@Diewertje11
Copy link
Collaborator Author

@victor-wildlife and @jannesgg , the pylint test is now running on all the kso_utils and there are quite some errors. (And I have solved some of them already) I started looking at the ipywidget error in the yolo_utils, since it says to not have a BBoxWidget and it indeed does not. In tutorial_utils we do 'from jupyter_bbox_widget import BBoxWidget' and that works indeed. But since this is wrong in the code in yolo_utils, I started looking at when that function gets used, and it does not. However, we do try to use a get_annotator from tutorial_utils, which does not have that (And pylint does not give an error about this, which I do not understand why. But maybe it encountered too many already?)

At least, this all made me question what we actually use from the annotator class in project.py. Can one of you look at this?

And feel free to resolve more of the errors we get. You can see them when you press on the details of the code quality test.

But if you work on this branch, please do make your own copy of it and make a draft pull request for that one, so that we do not get any conflicts. Then, if you have solved some of the errors, let me know and I can pull in the changes to this one.

@victor-wildlife
Copy link
Contributor

@Diewertje11 thanks for setting the pylint tests. It has already proven to be a really good idea to clean up our code.

I have resolved the errors on the server_utils in pull request #165 and will work on the other ones today.

@jannesgg might have a better understanding of the annotator class in project.py

Diewertje11 and others added 3 commits September 6, 2023 09:41
We need to specify in the .pylintrc that we use the new logging format
with {} and this we need to actually apply in all the loggin statements.
pylint highlighted issues with the duplication of the choose_workflows function so I removed the function from the project.py as we didn't use it in the tutorials and we already have one "choose_workflow" function in zooniverse_utils.
This was referenced Sep 6, 2023
@Diewertje11
Copy link
Collaborator Author

Great! All the issues pylint shows us are solved.
I looked more into the issue that I mentioned about that I found a bug that pylint is not showing us. It seems to be a bug in pylint, but we will look more into it. For now I added this information to Issue 241 in KSO, so that we can merge in these changes and start using pylint in the kso_utils. In that issue, I also listed 3 other things we need to take care off.

@jannesgg jannesgg self-requested a review September 6, 2023 14:10
Copy link
Contributor

@jannesgg jannesgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and ready to merge.

@jannesgg jannesgg merged commit 2dde0d2 into dev Sep 6, 2023
@jannesgg jannesgg deleted the pylint-test branch September 6, 2023 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants