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

Add Ast match of None value params #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

elcchan99
Copy link

None value parameters are ignored in extract_parameters since there are no AST match rule of it.

This PR adds the support on None value parameters.

@elcchan99 elcchan99 mentioned this pull request Mar 5, 2022
@takluyver
Copy link
Owner

The reason it doesn't recognise None is that there's a loose expectation that you replace parameters with the same type, and you can use the type to make simple decisions about how to get input - e.g. a spin box for an integer, or a checkbox for a boolean. This also encourages working with realistic defaults and running the notebook interactively.

But that's only a loose guideline, not a strict rule. I'm open to thinking differently about it. Can you say a bit about why you want to use None?

@elcchan99
Copy link
Author

there's a loose expectation that you replace parameters with the same type

I see. It makes sense to not accept None value if there is such expectation.

Can you say a bit about why you want to use None?

In my scenario, I have multiple runner environments of a single notebook file. The parameters are to configure how the notebook should behaviour differently. One param of them by default is None (running in local), but not None in other environments (maybe jupyterlab/ k8s). However, since the None params is excluded, the notebook is not executable in other environments.

But that's only a loose guideline, not a strict rule. I'm open to thinking differently about it

IMO, it is user's own practice to either prevent using None value as parameters or not. nbparameterise is the general tool that allows both possibility. ;)

@elcchan99
Copy link
Author

elcchan99 commented Mar 5, 2022

or in other words, I would see nbparameterise is only to patch needed parameters into new values instead of removing parameters (or any comments or python code) silently. Altho, I know the last one is not supported which could be a design choice. (e.g. Keep it simple & stupid)

@takluyver
Copy link
Owner

Sorry it's taken me so long to respond again.

In my scenario, I have multiple runner environments of a single notebook file. The parameters are to configure how the notebook should behaviour differently. One param of them by default is None (running in local), but not None in other environments (maybe jupyterlab/ k8s). However, since the None params is excluded, the notebook is not executable in other environments.

With #19 now merged, your foo = None will no longer be removed from the parameters cell when using nbparameterise - it will now only replace the bits it recognises - but it still won't recognise it as a parameter and let you replace it.

Is that OK? Or would it work to have a default value of the same type as the real value when the parameter is not meaningful? Like 0, -1 or '' (an empty string)? This is a simple workaround.

I'm still reluctant to recognise None as a parameter, because having a type with parameters means you can do things like building a form for them:

py_type_to_html_input_type = {
str: 'text',
int: 'number',
float: 'number',
}
def make_input_element(var):
if var.type is bool:
input_elm = Checkbox(var.name, var.value)
else:
input_elm = Input(py_type_to_html_input_type[var.type], var.name,
str(var.value))
if var.type is float:
input_elm.set_attribute('step', 'any')
return input_elm

Or use them to validate command-line arguments, as in nbclick.

This has got more complicated now that you can have lists and dicts as parameters, and we don't require them to be e.g. list-of-str. But it still seems like a useful idea for many purposes to have some simple types, so I'd rather not encourage people to use None as a default.

I guess type hints shift this a bit. We could e.g. recognise foo: Optional[int] = None as a parameter that expects an integer if you specify it at all. But this would be quite a shift, because so far nbparameterise has entirely ignored annotations. There would be a load of details to figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants