| msg48569 - (view) |
Author: Lisandro Dalcin (dalcinl) |
Date: 2005-07-04 18:03 |
| Greg Lielens submitted some time ago a patch [id 955928] about 'PyOS_Readline()' behavior with non-interactive tty's. Basically, there is no way to properly override 'PyOS_ReadlineFunctionPointer' as 'PyOS_Readline()' calls 'PyOS_StdioReadline()' when 'stdin' or 'stdout' are not tty's. A snippet of "Parser/myreadline.c": ... if (!isatty (fileno (sys_stdin)) | |
!isatty (fileno (sys_stdout))) rv = PyOS_StdioReadline (sys_stdin, sys_stdout, prompt); else rv = (*PyOS_ReadlineFunctionPointer)(sys_stdin, sys_stdout, prompt); ... Greg Lielens is completely right about the problem, but his patch is perhaps a bit crude, it also modifies "Python/bltinmodule.c" to solve the same issue with 'raw_input'. I think I have found a not so crude solution, and completely backward compatible. Basically, I moved 'isatty()' test from 'PyOS_Readline()' in file "Parser/myreadline.c" to 'call_readline()' in file "Modules/readline.c". In order to do that, I believe 'PyOS_StdioReadline' have to be added to file "Include/pythonrun.h". All those changes will not affect in any way the behavior in interactive sessions. Now 'PyOS_ReadlineFunctionPointer' can be properly overrode and users of 'readline' module will not see any change: in non-interactive tty's 'PyOS_StdioReadline()' will be called anyway. The problem in 'input' and 'raw_input' builtins remains, but its solution is not so clear to me, I think this part should not be touched right now. This patch passes 'Lib/test/regrtest.py' (of course, all those tests are non-interactive!). I use Python interpreter every day in the standard way and also in MPI-parallelized interactive sessions with this patch applied, and I never have any problem. I send my patch obtained with 'cvs diff -c' from current sources. |
|
| msg48570 - (view) |
Author: Gregory Lielens (greglielens) |
Date: 2005-08-06 19:14 |
| Logged In: YES user_id=1044428 Hi Lisandro, sorry that I did not follow this matter, we had other short term focus in our company and I had trouble finding time for it. Good that you subject this though, I have checked it fast and from what I understand it would have the same effect as my older patch... However, I used some features of my patch that are not present in yours: in Parser/myreadline.c, I removed the test about PyOS_ReadlineFunctionPointer being null to instead initialising it at creation to the correct value depending to system: This was important because it allows me in my specific MPI Readline function to use this defautl functio for the process 0...In general, it allows to re-use the default in the new Readline function, incrementing functionalities instead of re-implementing full functionality. You will have the same mechanism with the inclusion of PyOS_StdioReadline in the pythonrun API, but I personally find this solution less appealing: it increase the size of the API, and you do not have the correct readline on VMS...That's why I have done it like this, even if it may seems hackish ;) Of sourse manual has to be updated to say that PyOS_ReadlineFunctionPointer has a meaningfull defautl value...but it would also have to be updated in your approach, to tell about PyOS_StdioReadline... Regarding your modif of the Modules/readline.c, I have the feeling that your approach is cleaner...because It seems nicer to check for tty inside the new readline function, instead of installing the new readline function only if stdin/stdout are tty....You have to use PyOS_StdioReadline though, that would not be accessible if one does not add it to the pythonrun.h API, and that is not correct on vms system anyway (at least that's how I understand it...but I have no vms system to check ;) ). Maybe a better approach would be to cache the original PyOS_ReadlineFunctionPointer in the readline module, and re-use it if stdin/stdout are non-tty? This would be close to the approach I used for implementing my MPI readline: cache PyOS_ReadlineFunctionPointer and re-use it for modif... If this is considerd unclean, the approach putting PyOS_StdioReadline in the API can be kept, but It would have to be renamed something like PyOS_ReadlineFunction and be defined as PyOS_StdioReadline/vms__StdioReadline depending on the environment... Now for my patch on "Python/bltinmodule.c" to solve the same issue with 'raw_input', I think it was necessary to be able to use ipython in an interractive MPI session...Our embedded python interpreter had the possibility to use this (it gives a far nicier shell than the standard one), but I think It uses raw_input as entry mechanism...Could you test your patch to see if it work with this? The only drawback I see with this is that it could slow down a bit the parsing when using this function on a non-interractive file...but I doubt this is significative and could be optimized by keeping the call to PyOS_Readline but stripping out the prompt treatment in this case... We now are moving from an embbedded python interpreter to using a standard python...which make the acceptance of the patch more interresting: it would allows us to uses stock python interpreters > 2.x, no need to distribute our own interpretter anymore!!! :) |
|
|
| msg48571 - (view) |
Author: Gregory Lielens (greglielens) |
Date: 2005-08-06 19:35 |
| Logged In: YES user_id=1044428 Oups: please forget my previous message, I clicked on submit too fast and there is no correction/unsubmit option on sourceforge... Hi Lisandro, sorry that I did not follow this matter, we had other short term focus in our company and I had trouble finding time for it. Good that you subject this though, I have checked it fast and from what I understand it would have the same effect as my older patch... However, I still prefer some aspects of my patch over your’s ;) in Parser/myreadline.c, I removed the test about PyOS_ReadlineFunctionPointer being null: instead I initialize it at creation (to the correct value depending to system, vms or not): This initialization was important because it allows me to use this default function in my parallel Readline function... In general, ithe idea is to allow re-using the default in the new Readline function, incrementing functionalities instead of re-implementing full functionality. You will have the same mechanism with the inclusion of PyOS_StdioReadline in the pythonrun API, but I personally find this solution less appealing: it increase the size of the API, and you do not have the correct readline on VMS...That's why I have done it like this, even if it may seems hackish ;) Of course manual has to be updated to say that PyOS_ReadlineFunctionPointer has a meaningful default value...but it would also have to be updated in your approach, to tell about PyOS_StdioReadline... Well, a matter of taste I guess, but if the general opinion is that this initialization is unclean, I would then advice for renaming of PyOS_StdioReadline in the API to something like PyOS_ReadlineFunction, that would defined as PyOS_StdioReadline/vms__StdioReadline depending on the environment... Regarding your modify of the Modules/readline.c, I have the feeling that your approach is cleaner...because It seems nicer to check for tty inside the new readline function, instead of installing the new readline function only if stdin/stdout are tty....You have to use PyOS_StdioReadline though, again that would not be accessible if one does not add it to the pythonrun.h API (again that is not correct on vms system, but could be corrected taking my previous remark into account). An alternative approach would be to cache the original PyOS_ReadlineFunctionPointer in the readline module, and re-use it if stdin/stdout are non-tty? This would be close to the approach I used for implementing my MPI readline: cache PyOS_ReadlineFunctionPointer and re-use it for modif... Again, a matter of choice, I guess, more experienced python developers may have something to say about it. Now for my patch on "Python/bltinmodule.c" to solve the same issue with 'raw_input', I think it was necessary to be able to use ipython in an interactive MPI session...Our embedded python interpreter had the possibility to use this (it gives a far nicer shell than the standard one), but I think It uses raw_input as entry mechanism... Could you test your patch to see if it work also with ipython? If not then this modify has to stay I think... The only drawback I see with this is that it could slow down a bit the parsing when using this function on a non-interactive file...but I doubt this is significative and could be optimized by keeping the call to PyOS_Readline but stripping out the prompt treatment in this case... We now are moving from an embedded python interpreter to using a standard python...which make the acceptance of the patch all the more interesting for us: it would allows to use stock python interpreters > 2.x, no need to distribute our own interpreter anymore!!! :) |
|
|