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

Block fetching the settings for a plugin that is disabled. #7147

Merged
merged 20 commits into from Dec 6, 2019
Merged

Block fetching the settings for a plugin that is disabled. #7147

merged 20 commits into from Dec 6, 2019

Conversation

afshin
Copy link
Member

@afshin afshin commented Sep 3, 2019

References

Fixes #7107

As a side-effect of this work, I proposed we remove the concept of deferredExtensions for JupyterLab 2.0 on the weekly call.

Dependency: #7216

Code changes

  • Updates logic in index.js to make sure extensions that export a single plugin do not fall through the cracks and respect disabling/deferring.
  • The transformation timeout of the setting registry is shortened from 7 seconds to 1 second.
  • The setting registry plugin ignores deferredExtensions and disabledExtensions when it preloads the setting registry.
  • The setting registry plugin waits until app.restored and then attempts to load the settings for any plugins that did not preload, including deferredExtensions.
  • If there is a (1 second) timeout again for a specific plugin's settings and its settings schema has set the jupyter.lab.transform flag to true, there is a console warning that autoStart may have been set to false or that the plugin may be one of the deferredExtensions.

User-facing changes

N/A

Backwards-incompatible changes

N/A

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one minor comment inline.

While this is clearly a step in the right direction, I also start asking the question about what would happen if a transforming plugin has autostart: false, or is deferred, and is not required by another plugin. Since the transformers are typically (always?) registered in the activate function, I assume they would display the same behavior. I'm happy to leave the decision of whether to punt this or not to @afshin .

packages/services/src/setting/index.ts Outdated Show resolved Hide resolved
packages/services/src/setting/index.ts Outdated Show resolved Hide resolved
@afshin
Copy link
Member Author

afshin commented Sep 4, 2019

I'm not 100% convinced this is the right way to do this at all. Here are some of my concerns:

  1. Perhaps the jupyterlab_server settings endpoint should not return any settings for disabled plugins.
  2. The isDisabled logic is now being duplicated, once in @jupyterlab/services and once in index.js. Perhaps it belongs in PageConfig.

@afshin
Copy link
Member Author

afshin commented Sep 4, 2019

As for your question about transformers, they happen in the preload of the setting registry. That's populated by the list() method, so it should be okay.

I am conflicted about this approach because perhaps it is legitimate to disable a plugin but still load its preferences and use them.

We need to have a coherent rationale about this. That's why I've left this as a work in progress, because I'd like to raise the issue in our meeting today.

@vidartf
Copy link
Member

vidartf commented Sep 4, 2019

As for your question about transformers, they happen in the preload of the setting registry. That's populated by the list() method, so it should be okay.

How so? If (deferred or autorun==false) and not plugin_requested, the plugin will not register its transformers, but the list() method will still include that plugin's settings (the server doesn't know the status of the deferral). As a result, the page load will again be delayed by the transformation timeout of 10 seconds, as the registry is waiting for transformers that never appear.

I am conflicted about this approach because perhaps it is legitimate to disable a plugin but still load its preferences and use them.

Sure, let's try to brain storm some alternative solutions in today's call (written proposals also accepted 😉 ).

@afshin
Copy link
Member Author

afshin commented Sep 4, 2019

Yeah I suppose we could add a disabled flag to each plugin returned from the data connector and the setting registry could ignore transform on any disabled plugin. I'm not sure, this all sounds too complex. Perhaps it should be the way it is now or even one layer farther down with the server never even sending a disabled plugin's settings.

@afshin
Copy link
Member Author

afshin commented Sep 6, 2019

I think the Linux Usage failure is legitimate because I'm modifying the index.js files. But I'm not sure. This is still work in progress, regardless.

@afshin
Copy link
Member Author

afshin commented Sep 13, 2019

Okay, I think the reason that this check fails:

python -m jupyterlab.browser_check --core-mode

Is because it pulls PageConfig from the published @jupyterlab/coreutils. If this is the case, then the CI can't pass until we publish a new version of @jupyterlab/coreutils. Perhaps @saulshanabrook @jasongrout @vidartf know if I've understood this problem correctly?

@afshin afshin mentioned this pull request Sep 16, 2019
@afshin
Copy link
Member Author

afshin commented Nov 1, 2019

I am unsure what is happening here. The Linux usage test times out so I can't see log output. When I run the individual stages of the Linux Usage manually following along with ci_script.sh in my local environment, it passes.

Does anyone have any ideas how to un-stuck this?

@afshin afshin requested review from jasongrout and removed request for saulshanabrook November 4, 2019 15:13
Copy link
Member

@vidartf vidartf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afshin Sorry for being slow in reviewing this. This mostly looks good now, but I have some questions below.

}

async list(
query: 'active' | 'all' = 'all'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move this functionality to the services package itself? It seems as if it might have reuse value? That might also formalize it more, as we can add a docstring explaining that active implies that it is not disabled, and not deferred.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about putting it in services, but it seemed very specific to the functionality of the SettingRegistry and it seemed to me that the services package should be completely agnostic about what you're doing with the data being returned from the back-end.

Another place I considered putting this logic was as a query parameter to the back-end, which still remains a possibility (in which case, I'd agree with your suggestion to move this into services).

But for now, this seemed like the most unobtrusive thing to do because it seemed to localize the changes to the actual location of the concern. Do you disagree?

});

// If there are plugins that have schemas that are not in the setting
// registry after the application has restored, try to load them manually.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so we're clear: what is the purpose of the code that follows? This comment says what is happening, but not why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, if a plugin is deferred but not disabled and nobody has chosen to use it, its settings will not be editable by end users. This logic is meant to handle that scenario. It also handles the problem you originally pointed out if a deferred plugin has a transformation that never completes.

@vidartf vidartf merged commit ff4ee47 into jupyterlab:master Dec 6, 2019
afshin added a commit that referenced this pull request Dec 11, 2019
followup #7147: adds SettingConnector.save, reenables saving of user settings
@blink1073 blink1073 added this to the 2.0 milestone Jan 3, 2020
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 2, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug pkg:coreutils pkg:shortcuts status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delayed app load after disabling plugin with "jupyter.lab.transform" in schema
3 participants