-
Notifications
You must be signed in to change notification settings - Fork 567
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
WIP Recreate old behaviour of ExecutePreprocessor(kernel_name="") #886
Conversation
This is being introduced because we previously allowed users to pass "" to indicate that the kernel name should be exclusively inferred from the notebook itself. Doing so now overrides the default behaviour (which now defaults to infering the kernel name from the notebook itself if nothing is set). This is not a great pattern since it involves setting a parameter to a magic value to keep the default behaviour. This also issues a warning if you use the old way to do this.
In order to not use the default "python" behvaiour, I set the kernel to be "python3". However, that means this can only work if we have a python3 kernel available.
719a887
to
fc3dea1
Compare
Note: spatialaudio/nbsphinx#208 updates nbsphinx to use the preferred means of getting a kernel from notebook metadata. |
nbconvert/preprocessors/execute.py
Outdated
# Because of an old API, an empty kernel string should be interpreted as indicating | ||
# that you want to use the notebook's metadata specified kernel. | ||
# We use find_kernel_name to do this. | ||
if self.kernel_name == u"": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to do if not self.kernel_name:
? As a python developer I would expect that if "" is treated as falsy for a condition that None would be too. Or does the kernel_manager_class already handle None separately?
if not self.kernel_name:
self.kernel_name = find_kernel_name(self.nb)
if isinstance(self.kernel_name, string_types):
warnings.warn(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at it, I'm pretty sure None
would wouldn't have worked previously… but I still don't want to encourage passing people to do this regardless. So we'll want to warn no matter what type the value was.
The reason is that it's specified as a Unicode
traitlet so passing in None
would cause a validation error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So would checking for false kernel_name and declaring it deprecated in all cases make more sense from a function perspective then? I'm ok with merging if you feel strongly to just check for unicode string, it just had some code smell for something that might fail under similar situations -- like python version differences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python version difference was why I made it specifically a unicode string (since it's a Unicode() traitlet). I pushed a more general check and added a test to make sure that None
raises a TraitError
.
This is all really confusing! I don't see the necessity for dynamically changing the default value for Why don't you just completely get rid of Then, in if self.kernel_name:
kernel_name = self.kernel_name
else:
kernel_name = self.nb.metadata.get('kernelspec', {}).get('name', 'python') |
@mgeier I think I follow the suggestion. I'm still learning the nbconvert config system, so I don't know if there's side-effects that are important in assigning the default in the config and raising there for the AttributeError. But if there's not a secondary reason for this pattern, and nothing else would reuse this config then it would probably be simpler to move all the checks into |
I would recommend one of two approaches:
I don't have a strong preference, but the advantage of the first is that it relies less on traitlets magic, while the advantage of the second uses traitlets to ensure that |
Because we cannot guarantee that self.nb exists when kernel_name is set, this requires some… finagling to handle the empty string correctly. It would be better if we could lazily query the value rather than need to specify at the time of setting.
74c36bb
to
0d09e9d
Compare
Thanks for weighing in! I went with the second suggestion and ran into an issue. Because we set the kernel_name at the time of creating the Preprocessor, we run into an issue where we cannot set it to the correct value until the notebook is present to be able to query. If we could lazily load the value, that would work. I think we can have a blend of the two approaches, where the empty string coercion is done at the time of evaluation and we still include validation to check that something is either an empty string, That's going to be my next attempt at doing this. In talking with @MSeal about this we arrived at the idea that it could be helpful to have a method that explicitly defines these conventions so that they could be easily overwritten by subclasses without needing to change other parts of the machinery. In any case I'll get back to this tomorrow. |
Using It makes much more sense to keep Or am I missing something? |
FYI telling someone their design doesn't make sense comes off a bit aggressive, even when the suggested changes are valid suggestions. I would argue passing an empty string as a signal/default coercion behavior is not usually a good practice. Given this is just a backwards compatibility item, I wouldn't stress too much on exact details as we I believe we want to drop explicit |
There are certainly cases where it is not a good practice, but this case is not one of them. There is no reasonable interpretation of different intent whether a user passes an empty string or nothing at all for Sure, the original author could have chosen
That would be totally fine and probably useful, as long as this function takes the notebook in question as an argument or if this function is a method of an object that represents a notebook.
Please don't.
I'm sorry if I sounded aggressive, I didn't intend to. I'm not a native speaker, I didn't know of another way to say it and I didn't know that somebody could interpret this as aggressive. I've looked a bit more into this and found that the logical error was committed much earlier than this pull request or the commit it tries to fix. It is also quite confusing that there is a method named
[Update: it would probably be easier to understand if
Once this problem is resolved, our discussion here will become moot. But in the meantime, can we please keep |
Update: I just realized that In this case, all necessary state about the current notebook and the currently running kernel should be transported in the |
What is your motivation for this preference? |
Sorry I wasn't clear enough. In general I don't have a strong preference. As I said, I think both The reason why I mentioned If the default were changed to |
@mgeier doesn't |
@akhmerov No, using
|
I do agree with @akhmerov that None is the better pattern here and that from a python developer perspective Let's get M's change finalized to respect the pattern (even if it's not perfect for everyone) and redirect some of this energy into improving Preprocessor design overall. I think that's a more valuable use of time. |
Since there doesn't seem to be any progress here, I've created an alternative (simpler) PR here: #924. I'm fine with either solution, as long as the problem is fixed before the next |
Closing as request has been satisfied by related PRs |
This causes nbsphinx failures, for more details see: spatialaudio/nbsphinx#202 jupyter/nbconvert#878 jupyter/nbconvert#886 Should be fixed by the next nbconvert release (5.5).
This fixes #878 and spatialaudio/nbsphinx#202.
We had an API before that allowed passing an empty string as your kernel name, which it would then fall back on getting the kernel name from the notebook metadata.
This is now the actual default behaviour, and before this PR setting
kernel_name=""
would overwrite this default. This reintroduces the fall-back as well as a warning that this behaviour shouldn't be used (you can pass no value tokernel_name
and you will get the same behaviour).To ensure the same behaviour in both cases, it introduces the
find_kernel_name
API, which is then used in both the default and fallback mechanisms.This also introduces 2 tests and a new notebook file to be tested. In order to ensure that the functionality works I'm specifying a python3 kernel, which cannot be guaranteed to exist, so we have an expected fail in the case where there is no python3 kernel.