Feature req: Switch to turn Multiprocessing off

[madeddy]

Just a small idea. Do not know if this much work is(hopefully not).

If you run unrpyc through RenPy's own Python you get a ugly BANG, because it misses the multiprocessing module. Amongst other things... It would be useful if the multiprocessing can be turned of.

Greets

[jackmcbarn]

We already have -p 1 to turn multiprocessing off. Or do you mean it doesn't work even if you do that?

[CensoredUsername]

That would disable using it, but it's still imported which means it'd fail as soon as the module is loaded as it cannot find the import.

So to allow this to work we could import multiprocessing only when it's actually used. Which is possible, but we'd need to mock printlock with a dummy context manager.

[madeddy]

@jackmcbarn It's like he said in the first line. The import trys to find/use whats not there. RenPy's python is a very much neutered version compared to standard Py. @CensoredUsername I am not so sure using just one core with -p 1 does really disable multiprocessing. I interpret this as a reduction to a single core, however it runs still. I guess its a interesting question... Edit: I misinterpreted this obviously... and talked about the mp module and not the unrpyc code behavior with -p1.

Adding a switch to the parser and/or handling the import with the usual try-catch block would for me possible to do, but what to do with the printlock is beyond me. A dummy contextmanager sounds interesting.

[CensoredUsername]

I thought about this some more but honestly I'm not really interested in supporting running nonstandard python installs. We already offer one solution to people who cannot even install python. I spent a lot of time in the past moving unrpyc away from depending on ren'py internals due to how fragile it made everything, and it's extra effort for little gain. If people really want it they can make a pull request.

[madeddy]

... If people really want it they can make a pull request.

Like said i could try and write the code for this and make then a pull req. However how to write the "dummy context manager" for the printlock fly's over my head and the net didn't yield anything for it. Maybe a code example could help?

By the way: Maybe you could activate the new "discussions" feature in the repository settings so we do not need to discuss such ideas etc. in the issues section of unrpyc. Greets

[CensoredUsername]

Nah issues are fine.

The idea is simple, you've got to mock the lock object by replacing it with something that offers the same interfaces (which afaik are lock, unlock and the contextmanager API) consult python docs on what a context manager is.

[madeddy]

Hope commit to /dev branch was right. (dev is 1 behind master... huh) Please test and re. Works clean on my end. Note: I used the already existing dummy Lock from threading instead trying to write another.

p.s. I encountered some weird code where i believe its never reached. Discuss in new issue?

[CensoredUsername]

p.s. I encountered some weird code where i believe its never reached. Discuss in new issue?

What code?

[madeddy]

Nevermind. Made a mistake and confused two different vars for the same in line 164/167 in unrpyc.py.

However something else just for fun. Is the first condition needed or in the third already include like i believe? if args.write_translation_file and not args.clobber and path.exists(args.write_translation_file): Really not breaking anything this thing but i am curious.

[jackmcbarn]

However something else just for fun. Is the first condition needed or in the third already include like i believe? if args.write_translation_file and not args.clobber and path.exists(args.write_translation_file): Really not breaking anything this thing but i am curious.

If path.exists(None) returned False, then you'd be right, but it doesn't. It throws an exception.

[madeddy]

Ah. I expected i missed something. If i see this right: The None Type from the argparse default opt does this and i guess the first conditional "if args.write_translation_file" acts as a guard so the third check doesn't happen if not true.

I think with a empty str as default it should work without the first condition.

[CensoredUsername]

Using emptystring to indicate the lack of a variable being set is not type correct. None indicates the lack of something, emptystring implies the existence of the string, just that it's empty.

It's additionally not documented what os.path.exists returns when passed an empty path, and arguments can be made for both True and False. os.path.abspath("") tends to return a valid path matching the current working directory.

In general, I don't advice being smart by abusing sentinel values of types to indicate the absence of something when None is supposed to be the correct option for that in python. It leads to hard to modify code.

[madeddy]

Makes sense. Thanks for explaining some more.

It's additionally not documented what os.path.exists returns when passed an empty path...

Easy testet:

In [4]: os.path.exists(None)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-4-e9cd9da6fc69> in <module>()
----> 1 os.path.exists(None)

/usr/lib/python2.7/genericpath.pyc in exists(path)
     24     """Test whether a path exists.  Returns False for broken symbolic links"""
     25     try:
---> 26         os.stat(path)
     27     except os.error:
     28         return False

TypeError: coercing to Unicode: need string or buffer, NoneType found

In [5]: os.path.exists("")
Out[5]: False

abspath errors interestingly also on None type, but with:

In [6]: os.path.abspath(None)
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-6-d049dbc92460> in <module>()
----> 1 os.path.abspath(None)

/usr/lib/python2.7/posixpath.pyc in abspath(path)
    365 def abspath(path):
    366     """Return an absolute path."""
--> 367     if not isabs(path):
    368         if isinstance(path, _unicode):
    369             cwd = os.getcwdu()

/usr/lib/python2.7/posixpath.pyc in isabs(s)
     52 def isabs(s):
     53     """Test whether a path is absolute"""
---> 54     return s.startswith('/')
     55
     56

AttributeError: 'NoneType' object has no attribute 'startswith'

Though, its changed in py3 where both give us type errors. Just for the notes. I use nearly never os.path anymore. pathlib is it.

[CensoredUsername]

Easy tested:

Tested on a single version of a single operating system. I went digging in the cpython source code and after 4 redirects I still haven't found anything that actually checks this so it is likely dependent on the OS behaviour itself. Which means it could be different on any platform potentially.

Also that's not the point. It's bad to rely on undocumented behaviour just so you can do tricks in your code. Suddenly we cannot test anymore if an option is set with a simple "args.a is None" check. This makes the codebase hard to maintain.

And on Py3 you indeed should use path objects. Where pathlib.Path("") actually returns WindowsPath('.') which does actually exist. So you cannot do that trick in the first place. So lets just use the typesystem how it's intended to work, by using None to indicate absent values.

[madeddy]

Too wrap this somewhat up and for understanding: The discussion is by now not about changing anything, but out of curiosity, for fun and learning how stuff works.

Easy tested:

Tested on a single version of a single operating system.

Simply wrong. You need a new true-saying orb i think... After Jack said this in https://github.com/CensoredUsername/unrpyc/issues/106#issuecomment-770109694 i got really curious and tested this in three OS: On my Kubuntu 18.04.5 native, in virtualbox on 20.04, Win7. On 18.04/Win 7 with py2.7 and py3.8; in 20.04 admittedly just py3.8.

Forgot to add some infos: It's somewhat consistent in all OS and versions which i used how os.path.exists reacts. NoneType as path errors and empty string gives False. From py 2 to 3 we get just different errors.

Different for os.path.abspath behavior. Its a bloody massacre of inconsistency imho: os.path.abspath("") gives always the cwd. But os.path.abspath(None) gives in Win/Lx and py3 the same error, in Lx/py27 we get a different error and in Win/py27 no error, but the "cwd"! Yes

I went digging in the cpython source code and after 4 redirects I still haven't found anything that actually checks this so it is likely dependent on the OS behaviour itself. Which means it could be different on any platform potentially.

Hm. Hope we talk about the same: I did also search in the python src for os.path.exists behavior. For hours i did dig in January and believe i got at least bit about it: os.path.exists() is actually in genericpath defined. Inheritance goes like this: os(.path).py <-- nt-/posixpath.py <-- genericpath.py. "exists" calls there simply os.stat(path) and if a stat.result returns its a True. But how it knows its None or a empty string...did not find it or know not enough to understand it.

[CensoredUsername]

Different for os.path.abspath behavior. Its a bloody massacre of inconsistency imho:

It's what you get for passing a function an argument of the wrong type. NoneType and str are different, if something expects only str you shouldn't pass it NoneType. It might work due to implementation details, but you shouldn't count on it. It's like putting a chicken in a snake terrarium: I'm not sure why you'd even try to do it, and it will probably end badly in an unpredictable way. Due to dynamic typing it doesn't immediately count as an error but it's still not right.

"exists" calls there simply os.stat(path) and if a stat.result returns its a True.

Yeah I dug further there into what os.stat(path) does but that's where it becomes completely platform dependent.

Anyway simple rule, None is essentially a value that indicates the absence of a value. emptystring however is a valid value, it's just a string of length 0. If something says it expects a str as argument, it should always be passed an actual str instance or subclass of it, unless it also says that you can pass it None.

Dynamic typing allows you to be free in abusing some shortcuts in your own code, but I prefer to keep the rules straight as it keeps code maintainable. Especially for a codebase like this, which necessarily does some pretty insane things with types (magic.py is named like that for a reason), maintainability is important.

It also means that the inevitable port to python3 will be a bit easier.

[madeddy]

It's what you get for passing a function an argument of the wrong type. NoneType and str are different, if something expects only str you shouldn't pass it NoneType.

I am on your page there. Doing hacks do get around this gets just headache further don the road or for other dev's who want to understand whats happen there.

Yeah I dug further there into what os.stat(path) does but that's where it becomes completely platform dependent.

Yeah, i figured already you did go deeper. I cannot really read any C code, so i got in my head just questions marks with this files.

Dynamic typing allows you to be free in abusing some shortcuts in your own code, but I prefer to keep the rules straight as it keeps code maintainable. Especially for a codebase like this, which necessarily does some pretty insane things with types (magic.py is named like that for a reason), maintainability is important.

Makes sense to me and totally agreed. I do not like chaotic code. In a few things in life i am a pedant and from the look of it code is on of them. :wink:

It also means that the inevitable port to python3 will be a bit easier.

Hm, about this... i write to this in the py3 issue (#92) i made a while back. :innocent:

[madeddy]

Fixed by commits https://github.com/CensoredUsername/unrpyc/commit/7d3e42a9213b5939aa847e648a893f77bbf1fb53 and https://github.com/CensoredUsername/unrpyc/commit/d5286614b5dddd85bf1c72e24e9cb2ee58e50883