Optimize checking of supported args#152
Conversation
| @@ -1,6 +1,5 @@ | |||
| import getpass | |||
| import os | |||
| import pathlib | |||
There was a problem hiding this comment.
removed pathlib since it is not used
| try: | ||
| return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default) | ||
| except Exception: | ||
| return default |
There was a problem hiding this comment.
I don't think a try/except block is relevant here, I don't know of an error to be raised that we want that can be raised in this situation using os.getenv.
| try: | |
| return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default) | |
| except Exception: | |
| return default | |
| return os.getenv('JUPYTER_RSESSION_PROXY_WWW_FRAME_ORIGIN', default) |
Could you update the PR title or description to try capture this is now configurable via an environment variable? Whatever is put in the title will be visible via the changelog btw
There was a problem hiding this comment.
Ah, I've seen #148 now.
So this PR isn't really introducing this after all, its already there?
| try: | ||
| thread_pool_size = int(os.getenv('RSERVER_THREAD_POOL_SIZE', "")) | ||
| if thread_pool_size > 0: | ||
| cmd.append('--www-thread-pool-size=' + str(thread_pool_size)) | ||
| except Exception: | ||
| pass | ||
|
|
There was a problem hiding this comment.
If this is errorously configured, such as having RSERVER_THREAD_POOL_SIZE set to "asdf", I think it can make sense to error loudly instead. Of course, if its an empty string though, we shouldn't error, but we can check that without a try/except block.
I think maybe something like...
int(os.getenv('RSERVER_THREAD_POOL_SIZE') or "0") can do the trick, assuming we only want to configure --ww-thread-pool-size if its set to something else than 0.
3 participants
Hi all,
As @yuvipanda suggested in comments of this PR #151 (comment) I have split out a part that optimizes checking of supported arguments.
It would be great if we could merge this before upping the require jupyter-server-kernel. If that is possible I am happy to help with making separate PR for the other part that deals with support of unix_socket.