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.

Sign up for GitHub

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

Jump to bottom

DOCS-1866: Edit upload page for model framework ui update #2628

Merged
merged 9 commits into from
Mar 15, 2024
Merged

DOCS-1866: Edit upload page for model framework ui update #2628

merged 9 commits into from
Mar 15, 2024

Conversation

sguequierre
Copy link
Contributor

  • As far as I can tell this is the only place that needs to be changed?

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Mar 7, 2024
@viambot
Copy link
Member

viambot commented Mar 7, 2024 •
edited
Loading

Overall readability score: 55.58 (🟢 +0)

File Readability
upload-model.md 71.68 (🔴 -0.68)
View detailed metrics

🟢 - Shows an increase in readability
🔴 - Shows a decrease in readability

File Readability FRE GF ARI CLI DCRS
upload-model.md 71.68 54.12 8.95 10.5 9.63 6.15
  🔴 -0.68 🟢 +0.81 🟢 +0.18 🔴 -0.1 🔴 -0.76 🟢 +0.03

Averages:

  Readability FRE GF ARI CLI DCRS
Average 55.58 47.45 10.65 13.22 12.09 7.61
  🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0 🟢 +0
View metric targets
Metric Range Ideal score
Flesch Reading Ease 100 (very easy read) to 0 (extremely difficult read) 60
Gunning Fog 6 (very easy read) to 17 (extremely difficult read) 8 or less
Auto. Read. Index 6 (very easy read) to 14 (extremely difficult read) 8 or less
Coleman Liau Index 6 (very easy read) to 17 (extremely difficult read) 8 or less
Dale-Chall Readability 4.9 (very easy read) to 9.9 (extremely difficult read) 6.9 or less

JessamyT
JessamyT reviewed Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@JessamyT JessamyT left a comment

Choose a reason for hiding this comment

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

Ahh okay I think when I saw the ticket I'd thought there might be more places because I was thinking it was the "mlmodel" model of the vision service, as opposed to the "ML model service"--disregard what I said earlier!

assets/ml/upload-model.png Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can crop some of the white space off the right side of this to make it easier to see the details

docs/ml/upload-model.md Outdated
Comment on lines 55 to 58
{{% alert title="TFLite label files" color="tip" %}}
During Step 2, if you chose a TensorFlow Lite model framework, you add a `.txt` label file.
This should include the label names as you provided them in training, with one name per line.
{{% /alert %}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that you stipulate that this is for TFLite specifically versus the original "optional" bit. Is there anything else people need to know about specific model frameworks? Like, is there a reason we call this out but don't call out any other specifics?

What do you think of moving this into step 2 instead of down here? Could be a bullet under step 2, or just a second sentence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sguequierre I don't know if this comment was seen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JessamyT Sorry, I intended to address then it got hidden by a commit and I forgot 🤦‍♀️ I don't know about other model frameworks, I assumed we would get info from ML team if so. I can ask Steven

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with info from steven and moved into step 2 @JessamyT

Copy link
Collaborator

Choose a reason for hiding this comment

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

I figured that's what happened! :)

docs/ml/upload-model.md Outdated

Your model is now updated.

{{% alert title="TFLite label files" color="tip" %}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment above

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still outstanding--lmk what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok think addressed now!

JessamyT
JessamyT reviewed Mar 8, 2024
View reviewed changes
JessamyT
JessamyT reviewed Mar 8, 2024
View reviewed changes
@sguequierre sguequierre requested a review from JessamyT March 12, 2024 14:56
@sguequierre sguequierre mentioned this pull request Mar 12, 2024
Merged
JessamyT
JessamyT reviewed Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@JessamyT JessamyT left a comment

Choose a reason for hiding this comment

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

just a couple nits but also the outstanding comment from the first review about the note--this might not need action but not sure if you saw that comment

docs/ml/upload-model.md Outdated
Comment on lines 55 to 58
{{% alert title="TFLite label files" color="tip" %}}
During Step 2, if you chose a TensorFlow Lite model framework, you add a `.txt` label file.
This should include the label names as you provided them in training, with one name per line.
{{% /alert %}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sguequierre I don't know if this comment was seen?

docs/ml/upload-model.md Outdated

Your model is now updated.

{{% alert title="TFLite label files" color="tip" %}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still outstanding--lmk what you think

@sguequierre sguequierre requested a review from JessamyT March 13, 2024 18:31
JessamyT
JessamyT reviewed Mar 13, 2024
View reviewed changes
Copy link
Collaborator

@JessamyT JessamyT left a comment

Choose a reason for hiding this comment

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

Two more suggestions now that the details are added. Feel free to incorporate whichever parts of these comments you want. Only blocker is that I think the second set of steps got out of order?

@sguequierre sguequierre requested a review from JessamyT March 14, 2024 15:37
JessamyT
JessamyT approved these changes Mar 14, 2024
View reviewed changes
Copy link
Collaborator

@JessamyT JessamyT left a comment

Choose a reason for hiding this comment

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

LGTM so long as the semantics of uploading a model vs uploading a file are correct! (I made my last suggestions not knowing for sure--LGTM if you're confident!)

@viambot
Copy link
Member

viambot commented Mar 15, 2024

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/2628

@sguequierre sguequierre merged commit c866e61 into viamrobotics:main Mar 15, 2024
10 checks passed
@sguequierre sguequierre deleted the DOCS-1866/add-model-type branch March 15, 2024 15:41
sguequierre added a commit that referenced this pull request Mar 18, 2024
@sguequierre
DOCS-1866: Edit upload page for model framework ui update (#2628)
6060f29
sguequierre added a commit to sguequierre/docs that referenced this pull request Apr 25, 2024
@sguequierre
DOCS-1866: Edit upload page for model framework ui update (viamroboti…
acf269b 
…cs#2628)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JessamyT JessamyT JessamyT approved these changes

Assignees
No one assigned
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sguequierre @viambot @JessamyT