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

README update #2824

Closed
wants to merge 2 commits into from
Closed

README update #2824

wants to merge 2 commits into from

Conversation

abbycabs
Copy link

I took some inspiration from the tensorflow readme to:

  1. Point to a separate install guide. I highlighted readthedocs here (I can followup with a PR to update the docs to include an install guide)
  2. Separate out installing the current release with pip from using the pre-trained models on master. Note: I'm not sure that keeping the 0.6.1 models with 0.7 syntax here makes sense, but I'm leaving it in for now.

Based on my experience and feedback here: https://discourse.mozilla.org/t/installing-deep-speech-for-the-first-time-thinking-out-loud

* highlight readthedocs as main way to get started
* separate out installing the current release from using pre-trained models on master
* based on my experience here: https://discourse.mozilla.org/t/installing-deep-speech-for-the-first-time-thinking-out-loud
@community-tc-integration
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

README.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@kdavis-mozilla kdavis-mozilla left a comment

Choose a reason for hiding this comment

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

The change seems to imply there exists a released, trained model for MASTER which is not the case.

# Install DeepSpeech
pip3 install deepspeech

If you're using the **MASTER version** of DeepSpeech, you can get started using a pre-trained model:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to imply that there exists a pre-trained model for the MASTER version which is not the case.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure what to do with this part. We could either:

  1. delete these instructions
  2. reword to "If you're using the MASTER version of DeepSpeech, here is the syntax for using a pre-trained model:"

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this seems more confusing than it was at the start.

Now this documentation has to juggle two versions of the code. Where previously it had a big warning that said go to readthedocs for the latest stable version, otherwise stay here for Master.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks Kelly - I'm trying to wrap my head around why this block of code exists? It mixes installing the stable version, using 0.6.1 models, and master syntax. When will someone be able to run this code?

I'm trying to separate it out into chunks that are usable, but that seems to be creating its own problems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When will someone be able to run this code?

This is accurate at release time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@acabunoc Did you tripped over this because of the amount of docs, or because it's unclear, or because of something else ?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for asking @lissyx --

I made the assumption that the first block of code on your landing page would be a good place to get started. I understand that there are instructions to look elsewhere, and I eventually saw that and moved to the docs. However, I will not be the only person making this assumption. Most thriving open source projects will have a usable block of code on their landing page, if they have code there. I'd love to see more developers stepping into this project and immediately feeling comfortable.

Tiny changes like this can make a huge different in community engagement and go long way to show new contributors that their presence is welcome here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Most thriving open source projects will have a usable block of code on their landing page, if they have code there. I'd love to see more developers stepping into this project and immediately feeling comfortable.

Well, I share that. At the same time, everytime I'm looking at some docs from a master branch, I know it might fail, and if it does, I look for something that is stable.

I think we all on the team assumed this was the default behavior of people. Is this wrong ? Do people don't care about matching-version documentations ?

I don't see how we can ensure and provide always-working master branch + instructions, especially given a release is made when a new model is ready.

I made the assumption that the first block of code on your landing page would be a good place to get started. I understand that there are instructions to look elsewhere, and I eventually saw that and moved to the docs.

The obvious solution would be to keep "stable" instructions on top, but then it is also confusing to people in other ways. And it's also going to be much more messy to mix.

I honestly have no idea how we can fix everything at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't provide always working master branch instructions. As it is, our master branch is wrong most of the time. It's only correct in the time between a stable release and the first breaking change. As far as I can see, the only benefit our policy brings is to reduce the number of changes we have to do at release time (just update the model URL). Personally I'm fine with leaving the master instructions matching the latest stable release.

Another possibility is to create release branches in addition to release tags, and then switching the GitHub default branch after a release. That way when users come to the repo the stable instructions are shown, and we make it so that only contributors need to do the extra work of switching versions. I guess people who know enough to change the code and send PRs also know enough that changing versions won't be a hassle.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks both!

Well, I share that. At the same time, everytime I'm looking at some docs from a master branch, I know it might fail, and if it does, I look for something that is stable.

I think we all on the team assumed this was the default behavior of people. Is this wrong ? Do people don't care about matching-version documentations ?

In many cases, I think you're right. But this is also the main landing page for your project -- not just some docs. Small barriers here can signal that that you have your own short-hand on how you work as a team & you're not ready for outside contributors.

I agree that people experienced in open source & code can easily get over this. But I would hope that this project could be welcoming to more than the existing open source crowd (which has its own diversity problems).

@reuben - huge thanks for bringing up a couple solutions. Both are great. The team knows more about your workflow/community to know what would work best here. Let me know if I can help at all.

@reuben
Copy link
Contributor

reuben commented Apr 28, 2020

@acabunoc I just landed a pull request slimming down the GitHub README to point people to readthedocs.io, which contains versioned documentation and by default shows docs for the latest stable release. I've also moved the demo section from the README into the readthedocs index page: https://deepspeech.readthedocs.io/en/master/

@abbycabs
Copy link
Author

Huge thanks @reuben! I'll go ahead and close this.

@abbycabs abbycabs closed this Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation waiting-on-reporter Waitiing on more informations from reporter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants