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

proper check for filename before calling subprocess #1485

Open
prakhar2b opened this issue Jul 14, 2017 · 10 comments
Open

proper check for filename before calling subprocess #1485

prakhar2b opened this issue Jul 14, 2017 · 10 comments
Labels
bug Issue described a bug difficulty medium Medium issue: required good gensim understanding & python skills good first issue Issue for new contributors (not required gensim understanding + very simple) Hacktoberfest Issues marked for hacktoberfest

Comments

@prakhar2b
Copy link
Contributor

prakhar2b commented Jul 14, 2017

Based on discussion here, we should properly check for filename before process calls in utils.check_output(), and raise more intuitive exception and error message.

cc @jayantj

@tmylk
Copy link
Contributor

tmylk commented Jul 23, 2017

@tmylk
Copy link
Contributor

tmylk commented Jul 23, 2017

The cpython check_output code https://github.com/python/cpython/blob/master/Lib/subprocess.py

@AverageS
Copy link

@tmylk @prakhar2b
#1501
It should fix the problem

@menshikh-iv menshikh-iv added bug Issue described a bug difficulty medium Medium issue: required good gensim understanding & python skills labels Oct 2, 2017
@menshikh-iv menshikh-iv added the good first issue Issue for new contributors (not required gensim understanding + very simple) label Jan 25, 2018
@pratikmjoshi
Copy link

@menshikh-iv Can I go ahead with this?

@menshikh-iv
Copy link
Contributor

@pratikmjoshi of course

@pratikmjoshi
Copy link

pratikmjoshi commented Mar 5, 2018

@menshikh-iv Just to clarify, fix #1501 did not solve this issue, right? Am I supposed to add an exception and error message for an incorrect filepath to the FastText directory, and should it indicate that the path be changed to the executable? What should the error message look like? I assume that I'll have to take care of the windows case as well.

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Mar 5, 2018

@pratikmjoshi #1501 solved this issue, but partially

  • the solution should work on main OS system (Linux, MacOSX, Windows)
  • the idea is simple: before running anything, check that this path exists & executable

@pratikmjoshi
Copy link

pratikmjoshi commented Mar 5, 2018

@menshikh-iv Sorry, but I've been getting a bit confused with the code. In utils.py, is a function like this for path checking ok? The code is basically from #1501 , but I've added a check for amending the path depending on the platform. What will be the Windows equivalent of /usr/bin/ (I've just put C:\Users\ ,pretty sure that its incorrect)? Am I missing anything else?

screenshot from 2018-03-06 04-34-39

And at check_output....

screenshot from 2018-03-06 04-28-36

@pratikmjoshi
Copy link

@menshikh-iv After making the necessary changes, the respective error messages are raised as below:

screenshot from 2018-03-06 04-40-03

screenshot from 2018-03-06 04-40-17

However, after entering the right path, I'm still getting this error on the notebook. Any advice?

screenshot from 2018-03-06 04-43-02

screenshot from 2018-03-06 04-43-05

@menshikh-iv
Copy link
Contributor

@pratikmjoshi about path (/usr/bin, etc): they can be many varieties only on Linux + user can have some custom path, and we have not started with other OS. IMO this way doesn't look like the right way.

Also, sometimes, wappers want to path to "folder" (and have hard-coded executable), please check, how it works for all wrappers (it's not so simple).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue described a bug difficulty medium Medium issue: required good gensim understanding & python skills good first issue Issue for new contributors (not required gensim understanding + very simple) Hacktoberfest Issues marked for hacktoberfest
Projects
None yet
Development

No branches or pull requests

5 participants