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

Yolo docs #1715

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Yolo docs #1715

wants to merge 10 commits into from

Conversation

Sam948-byte
Copy link
Contributor

This adds documentation for training a yolov8 to be deployed to an opi.

@Sam948-byte Sam948-byte requested a review from a team as a code owner January 13, 2025 00:22
@mcm001
Copy link
Contributor

mcm001 commented Jan 13, 2025

Did you write the notebook? If not, give attribution as required by the license it was released under.

@Sam948-byte
Copy link
Contributor Author

@mcm001 so the notebook was released under GPL 3. Since photonvision is already released under GPL 3, it's not necessary to add any further licensing correct?

Copy link
Contributor

@gerth2 gerth2 left a comment

Choose a reason for hiding this comment

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

A few things to think about before we publish. But overall theme - some docs are better than none.

@@ -35,7 +35,7 @@ Photonvision will letterbox your camera frame to 640x640. This means that if you

## Training Custom Models

Coming soon!
There is a [Jupyter Notebook](https://github.com/PhotonVision/photonvision/blob/main/scripts/yolov8_to_rknn.ipynb) that contains all of the code necessary to train a yolov8 model for upload.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discord - this should be permalinked - IE, the link contains the sha1 of the commit referenced.

https://github.com/PhotonVision/photonvision/blob/54778b4e1c6ae0f6ccd3d663384ca250b0b119a5/scripts/yolov8_to_rknn.ipynb is one way of doing it, though that links to a PR of the file.

I don't actually know if/how to permalink before the PR is approved, I'd ask for a quick investigation to see if we can hit this in one shot. If not, it's a followup after the new file is merged to main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

permalink I think it has to be committed first

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks categorically correct today. Haven't run personally.

General thought: what's our confidence this will remain right over the next year or two?

Expanding: how many things are just "pip installed" to the latest, or pulled from a repo as latest (and not a specific version)?

I'm likely not going to fight as-is, but might be asking for more hedging in the link into this document, or comments at the top indicating the last time it was tried. I say this after having been burned by somethign similar last year - a jupyter notebook that worked in 2023 stopped working in 2024 due to underlying changes in airockchip's content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be good? It worked from last year to this year. We can also set it up so the notebook checks out the current commit from the repos.

},
"source": [
"## Downloading the dataset\n",
"Input your Roboflow API key below. It can be obtained [here](https://app.roboflow.com/settings/api).\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I do like this comment specifically, it will be a key thing users need to supply up front.

Thinking through generally: is this the only thing? Does it make sense to document this in the .rst as well, to give a very specific "before running the notebook..." procedure of what needs to be prepped?

Again some docs are better than none, so I"m ok saying "next PR" for more details - though I would advise confirming the admonition that this is still "some knowledge required" - I worry most about the team that's just learned about ML, decided they want to use it on their bot, and come to PhotonVision with the hopes that the simplicity they've seen in other areas will carry forward to ML. Providing enough admonition to get them to think twice before assuming PV has solved ML in general for whatever they want is really my goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a scary warning

@Sam948-byte Sam948-byte marked this pull request as draft January 13, 2025 16:32
@steve-carpenter
Copy link

Sorry to hijack this thread- but a couple comments (just got this to work):

I switched to rknn-toolkit2 (previous version threw an error for be during the onnx to rknn converstion; this is the latest version and worked successfully for me)

!wget https://github.com/airockchip/rknn-toolkit2/raw/refs/heads/master/rknn-toolkit2/packages/x86_64/rknn_toolkit2-2.3.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
!pip install ./rknn_toolkit2-2.3.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

additionally, looks like we were switching to a particular commit for model zoo, seems like main works out of the box:

!git clone https://github.com/airockchip/rknn_model_zoo/
%cd rknn_model_zoo
%cd examples/yolov8/python

Lastly, perhaps would be advantageous to change output_model to standard format PV is expecting. Maybe something like?
output_model = f"{root_path}/{game}-{image_size}-{image_size}-{model}.rknn"

@Sam948-byte
Copy link
Contributor Author

Sam948-byte commented Jan 16, 2025

So I do agree that main does work out of the box for both rknn-toolkit-2 and the rknn_model_zo, but the reason we recommend specific commits is because these are known goods that won't change in the future. If the developers should introduce some sort of breaking feature, we want to ensure that it doesn't get pulled, which would occur if we're just grabbing the most recent version. I can double-check the rknn-toolkit-2 version that's being used here, but I included it as it was working on my machine. If it doesn't work on yours, I can look into upgrading it. Finally, I do agree that it could be advantageous to rename, but teams who are training their own models will likely want to name them based on their preferences, and having a set name could result in overwriting prior models, so I don't think that's going to be implemented at the time.

I'm glad this guide was able to help you train your own model, and if you're looking for a dataset, or have any data that you would like to contribute, our dataset is currently hosted here.

@steve-carpenter
Copy link

steve-carpenter commented Jan 16, 2025

Thanks for prompt response!

FYI- For now, I used this public dataset: https://universe.roboflow.com/team-8847/reefscape-collation/

@Sam948-byte
Copy link
Contributor Author

Funnily enough, that's our old dataset. We're migrating to the new location due to upload limits.

@steve-carpenter
Copy link

steve-carpenter commented Jan 16, 2025

Is that dataset you linked public? Link doesn't seem to work (redirects to my projects list).

Nvm got it.

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.

4 participants