More thorough python dependency checking #1876
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After extras/check_dependencies.sh is now run, the following is ensured: - python2.7 is installed and the binary python2 exists (if not, a link to python2.7 is added) - python3 is installed - python links to python2.7. If it is not the case a symlink is added that is placed early on the path This is basically the first step in resolving kaldi-asr#1592 and does the basic checking for python3 as it is already used in some places (I saw it e.g. mentioned in kaldi-asr#1707)
|
Seems reasonable but @jtrmal should check as it interacts with env.sh. |
jtrmal
requested changes
Sep 8, 2017
tools/extras/check_dependencies.sh
Outdated
| mkdir -p $PWD/python | ||
| echo "$0: python2.7 is installed, but the python2 binary does not exist. Adding this to tools/env.sh" | ||
| ln -s $(which python2.7) $PWD/python/python2 | ||
| echo "export PATH=$PWD/python:\${PATH}" >> env.sh |
There was a problem hiding this comment.
you are creating python2 link, but we usually use shebang python
it would be OK I guess in case when you would be working on PR that changes the shebangs to python2 -- without that I'm not sure it would fix any issue
There was a problem hiding this comment.
Yes, my thought has been that we switch to use python2 shebangs, either at once or gradually. I just did not want to include it in this same PR, but that would be coming.
| if ! which python3 >&/dev/null; then | ||
| echo "$0: python3 is not installed" | ||
| add_packages python3 | ||
| pythonok=false |
There was a problem hiding this comment.
I see you are adding requirement for python3 -- is this necessary? I remeber @danpovey has a couple of scripts for LM stuff, but those were not merged yet.
tools/extras/check_dependencies.sh
Outdated
| echo "$0: python is not installed (we need python 2.7)" | ||
| add_packages python2.7 python python2.7 | ||
| echo "$0: WARNING python 2.7 is not the default python. We fixed this by adding a correct symlink more prominently on the path." | ||
| echo "$0: If you really want to use python3 as default, remove $PWD/python/python and add an empty file $PWD/python/stubborn" |
There was a problem hiding this comment.
this is a confusing message imho.
creating that stubborn file does not really imply using python3 -- it implies using whatever is linked to "python"
There was a problem hiding this comment.
You are absolutely right. I created the "stubborn" hook for some testing locally. I'm thinking I could remove that, as those that would want to be stubborn can just edit this script themselves. Does that sound better?
|
Regarding the requirement for python3-- it's true that we don't require it
yet, but we will soon, and I'd rather not have to rewrite the dependency
script at that time, so it's OK with me to add this dependency now.
Dan
…On Fri, Sep 8, 2017 at 3:57 PM, jtrmal ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In tools/extras/check_dependencies.sh
<#1876 (comment)>:
> +fi
+
+if ! which python3 >&/dev/null; then
+ echo "$0: python3 is not installed"
+ add_packages python3
+ pythonok=false
+fi
+
+(
+#Use a subshell so that sourcing env.sh does not have an influence on the rest of the script
+[ -f ./env.sh ] && . ./env.sh
+if $pythonok && ! which python2 >&/dev/null; then
+ mkdir -p $PWD/python
+ echo "$0: python2.7 is installed, but the python2 binary does not exist. Adding this to tools/env.sh"
+ ln -s $(which python2.7) $PWD/python/python2
+ echo "export PATH=$PWD/python:\${PATH}" >> env.sh
you are creating python2 link, but we usually use shebang python
it would be OK I guess in case when you would be working on PR that
changes the shebangs to python2 -- without that I'm not sure it would fix
any issue
------------------------------
In tools/extras/check_dependencies.sh
<#1876 (comment)>:
> @@ -87,28 +87,40 @@ if ! which awk >&/dev/null; then
add_packages gawk gawk gawk
fi
-if which python >&/dev/null ; then
+pythonok=true
+if ! which python2.7 >&/dev/null; then
+ echo "$0: python2.7 is not installed"
+ add_packages python2.7
+ pythonok=false
+fi
+
+if ! which python3 >&/dev/null; then
+ echo "$0: python3 is not installed"
+ add_packages python3
+ pythonok=false
I see you are adding requirement for python3 -- is this necessary? I
remeber @danpovey <https://github.com/danpovey> has a couple of scripts
for LM stuff, but those were not merged yet.
------------------------------
In tools/extras/check_dependencies.sh
<#1876 (comment)>:
> - status=1
- else
- echo "$0: python 2.7 is not installed"
- add_packages python2.7 python python2.7
- fi
- fi
-else
- if which python2.7 >&/dev/null || which python2 >&/dev/null ; then
- echo "$0: python 2.7 is not the default python. You should either make it"
- echo "$0: default or create an bash alias for kaldi scripts to run correctly"
- status=1
- else
- echo "$0: python is not installed (we need python 2.7)"
- add_packages python2.7 python python2.7
+ echo "$0: WARNING python 2.7 is not the default python. We fixed this by adding a correct symlink more prominently on the path."
+ echo "$0: If you really want to use python3 as default, remove $PWD/python/python and add an empty file $PWD/python/stubborn"
this is a confusing message imho.
creating that stubborn file does not really imply using python3 -- it
implies using whatever is linked to "python"
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1876 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADJVuw8CzBpCTG6LmD9NVmhXgYqU4Djlks5sgZwQgaJpZM4PQ65K>
.
|
|
I don't personally object to the idea - but naming it ".use_default_python"
sound better to me.
Also please verify what happens when you run the depends script once,
create the file afterwards and then run it again - in that case it
shouldn't fail and perhaps the links should be removed - maybe it does that
already, I didn't check.
Or the message should inform the user about the necessity of removing the
links manually
Y.
…On Sep 9, 2017 3:20 AM, "Peter Smit" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tools/extras/check_dependencies.sh
<#1876 (comment)>:
> - status=1
- else
- echo "$0: python 2.7 is not installed"
- add_packages python2.7 python python2.7
- fi
- fi
-else
- if which python2.7 >&/dev/null || which python2 >&/dev/null ; then
- echo "$0: python 2.7 is not the default python. You should either make it"
- echo "$0: default or create an bash alias for kaldi scripts to run correctly"
- status=1
- else
- echo "$0: python is not installed (we need python 2.7)"
- add_packages python2.7 python python2.7
+ echo "$0: WARNING python 2.7 is not the default python. We fixed this by adding a correct symlink more prominently on the path."
+ echo "$0: If you really want to use python3 as default, remove $PWD/python/python and add an empty file $PWD/python/stubborn"
You are absolutely right. I created the "stubborn" hook for some testing
locally. I'm thinking I could remove that, as those that would want to be
stubborn can just edit this script themselves. Does that sound better?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1876 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisX0Hk45hwyVKORjdG4HMjQPyXQRfDks5sgjxagaJpZM4PQ65K>
.
|
|
Sorry -- the previous reaction should go to the "stubborn" thread -- Seems like github does not handle nicely when replying via e-mail. |
Fixing the "default python" messages and behavior according to reviews by Yenda
|
There was only the removal message on the first time running the script. I now updated it to actually removing the symlink when python/.use_default_python is present. I tested all scenarios I could think of for the python==python3.x case. I do not give a message when .use_default_python exists, but I'm not certain that is necessary? The only thing "untested" in real situations is the "/usr/bin/python2 doesn't exist" case. Can someone else test that? |
|
LGTM |
kronos-cm
added a commit
to kronos-cm/kaldi
that referenced
this pull request
Sep 16, 2017
* 'master' of https://github.com/kaldi-asr/kaldi: (43 commits) [src,scripts,egs] Transfer learning for ASR with nnet3 (kaldi-asr#1633) [src,scripts,egs] Attention modeling, with example scripts (kaldi-asr#1731) [src] Fix bug in block matrix addition (thanks: Sidhi Adkoli). [egs] Fix inconseqential input-checking bug in Swbd example script (kaldi-asr#1886) [build] dependency-check: that python2.7 and python3 exist and 2.7 is default (kaldi-asr#1876) [scripts] A cosmetic change to info messages in chain training (kaldi-asr#1880) [doc] Keep tutorial code up to date (thanks: Luwei Yang) [scripts] Bug-fix in long-utterance-segmentation script (thanks: Armin Oliya) (kaldi-asr#1877) [egs] Fixed some issues in the multilingual BABEL example scripts (kaldi-asr#1850) [build] Cosmetic fix in Makefile Remove memory leaks and unused variables (when CUDA is not enabled) (kaldi-asr#1866) [scripts] Fix default for egs.cmd in nnet3 training scripts (kaldi-asr#1865) [doc] Fix to how documentation is built (thanks: David van Leeuwen) [scripts] Add --decode-extra-opts in steps/decode.sh (required for speech activity detection scripts) (kaldi-asr#1859) [src] Adding documentation for lattice discriminative training functions (kaldi-asr#1854) [src] Typo fixes in documenation. (kaldi-asr#1857) [egs] Update to score.sh in fisher_swbd setup, allow --iter option (kaldi-asr#1853) [scripts] bug-fix in TFRNNLM rescoring script (no 'ark' needed for unk.probs file) (kaldi-asr#1851) [src] Remove repeated parameter documentation. (kaldi-asr#1849) [egs] Aspire example scripts: Update autoencoder example to xconfig (kaldi-asr#1847) ...
Skaiste
pushed a commit
to Skaiste/idlak
that referenced
this pull request
Sep 26, 2018
galv
pushed a commit
to galv/kaldi
that referenced
this pull request
Dec 10, 2022
…appears to be requiring both python 2 and 3 for the foreseeable future. See pull request discussion in Kaldi public repo: kaldi-asr#1876, kaldi-asr#1592
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After extras/check_dependencies.sh is now run, the following is ensured:
This is basically the first step in resolving #1592 and does the basic checking for python3 as it is already used in some places (I saw it e.g. mentioned in #1707)
This has been tested on our linux machines and clusters. I do not have access to a Mac, so it would need to be verified to be working there.