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

Automatically install ds_ctcdecoder in setup.py #2871

Merged
merged 2 commits into from
Apr 1, 2020
Merged

Conversation

reuben
Copy link
Contributor

@reuben reuben commented Mar 31, 2020

No description provided.

@reuben reuben requested a review from lissyx March 31, 2020 15:57
setup.py Outdated Show resolved Hide resolved
@@ -122,25 +122,3 @@ verify_bazel_rebuild()
exit 1
fi;
}

# Should be called from context where Python virtualenv is set
verify_ctcdecoder_url()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe worth adding unit test coverage of the similar feature we have in setup.py ? Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to test setup.py. I guess installing the package and then trying to import the decoder dependency to see if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still not going to test the entire URL computation part that users will use, though.

doc/TRAINING.rst Outdated
@@ -46,22 +46,15 @@ Install the required dependencies using ``pip3``\ :
.. code-block:: bash

cd DeepSpeech
pip3 install -e .
pip3 install --upgrade pip wheel setuptools
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need latest version ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, but depending on the OS/distro/Python version fresh virtualenvs come with older versions so for users of the training code I added this instruction to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example some versions are incompatible with TF 1.15 wheels.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but forcing always latest version can bring its own issues. I'd feel better if we could stick to well known versions, like we do at several other places.

Copy link
Collaborator

@lissyx lissyx left a comment

Choose a reason for hiding this comment

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

LGTM with green tests :)

@lock
Copy link

lock bot commented May 5, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants