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

Enable test_word2vec_stand_alone_script by using sys.executable for python #3299

Merged
merged 1 commit into from
Apr 15, 2022
Merged

Enable test_word2vec_stand_alone_script by using sys.executable for python #3299

merged 1 commit into from
Apr 15, 2022

Conversation

pabs3
Copy link
Contributor

@pabs3 pabs3 commented Feb 26, 2022

No description provided.

@pabs3 pabs3 force-pushed the path-to-python-binary branch 6 times, most recently from 8cb676f to 5845be3 Compare February 26, 2022 09:45
piskvorky
piskvorky requested changes Feb 26, 2022
View reviewed changes
Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

Code style.

gensim/test/test_word2vec.py Outdated Show resolved Hide resolved
@pabs3 pabs3 force-pushed the path-to-python-binary branch from 5845be3 to 7cb6050 Compare February 26, 2022 23:26
@pabs3
Copy link
Contributor Author

pabs3 commented Feb 26, 2022

I note that this newly enabled test is failing, but I have no idea what the error is about, any thoughts?

@pabs3 pabs3 requested a review from piskvorky February 26, 2022 23:33
@piskvorky
Copy link
Owner

piskvorky commented Feb 27, 2022 •
edited
Loading

The external command is expected to print 0 to stdout; instead, it prints nothing. So the test fails.

I'm not really sure why it expects 0 – could you please check by running it manually? What does it actually print and why?

@pabs3 pabs3 force-pushed the path-to-python-binary branch from 7cb6050 to 3a2cffe Compare March 20, 2022 02:14
@piskvorky
Copy link
Owner

@pabs3 did you find out the cause of the 0-vs-nothing difference?

piskvorky
piskvorky requested changes Mar 20, 2022
View reviewed changes
gensim/test/test_word2vec.py Outdated Show resolved Hide resolved
@piskvorky piskvorky added this to the Next release milestone Mar 20, 2022
@pabs3
Copy link
Contributor Author

pabs3 commented Apr 2, 2022 via email

@pabs3 pabs3 force-pushed the path-to-python-binary branch from e9a404f to c15d1a9 Compare April 2, 2022 06:15
@pabs3
Copy link
Contributor Author

pabs3 commented Apr 2, 2022

I did some digging just now and I can't find a time when gensim.scripts.word2vec_standalone ever output an integer like the test suggests it does. Back in 2016 the test for it was failing and got disabled in commit 055cd7d in order to make a release and it was originally added with the check for 0 in PR #538.

@pabs3 pabs3 force-pushed the path-to-python-binary branch from c15d1a9 to 3ad9841 Compare April 2, 2022 06:45
@pabs3
Copy link
Contributor Author

pabs3 commented Apr 2, 2022

I've switched the test to check for b'' instead of '0'.

@piskvorky
Copy link
Owner

piskvorky commented Apr 2, 2022 •
edited
Loading

Hm, that PR was closed, not merged.

In any case, if you fixed a bug that's great, thanks. I see the CI is green now.

@piskvorky piskvorky merged commit de9ee81 into piskvorky:develop Apr 15, 2022
@piskvorky
Copy link
Owner

Thanks again @pabs3 !

@pabs3 pabs3 deleted the path-to-python-binary branch May 4, 2022 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piskvorky piskvorky piskvorky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pabs3 @piskvorky