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

Install ipython dagster kernel if not present #1047

Merged
merged 3 commits into from
Apr 3, 2019
Merged

Conversation

mgasner
Copy link
Contributor

@mgasner mgasner commented Mar 26, 2019

Resolves #908

@mgasner mgasner self-assigned this Mar 26, 2019
@mgasner mgasner requested review from puma314 and schrockn March 26, 2019 23:40
@coveralls
Copy link

coveralls commented Mar 26, 2019

Pull Request Test Coverage Report for Build 105762

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.092%

Totals Coverage Status
Change from base Build 105070: 0.0%
Covered Lines: 7797
Relevant Lines: 9163

💛 - Coveralls

@schrockn
Copy link
Member

we should document what is going on here. this is only for the development environment correct? we don't assume the "dagster" kernel name in the dagstermill core

@mgasner
Copy link
Contributor Author

mgasner commented Mar 27, 2019

Dagstermill tests invoke notebooks that look for an ipython kernel called dagster -- if this is not already present, then the tests fail with a cryptic error (#839). In the status quo, this will generally happen on first download / install -- with this diff, that kernel is created if it is not already present before tests run.

@mgasner
Copy link
Contributor Author

mgasner commented Apr 3, 2019

ping @schrockn

@schrockn
Copy link
Member

schrockn commented Apr 3, 2019

So we are committing to the fact that the user must have a hardcoded kernel named dagster in the system?

@mgasner
Copy link
Contributor Author

mgasner commented Apr 3, 2019

I could look for a kernel named dagster, and if we don't find one, then overwrite the kernel in the test notebooks with a kernel that they do have (?) -- then rewrite it at test cleanup.

@mgasner
Copy link
Contributor Author

mgasner commented Apr 3, 2019

nteract/papermill#338

@schrockn
Copy link
Member

schrockn commented Apr 3, 2019

Where would be a good spot to communicate to the user what we are doing. Somewhere that prints out "We have automatically created a kernel named "dagster". <Insert some explanation of what a kernel is. If you have your own kernels you want to use, please edit the notebook to do it"

Or something of the sort.

@mgasner
Copy link
Contributor Author

mgasner commented Apr 3, 2019

I've added a warning... the nice thing about warnings is they do print by default in pytest.

Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

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

👍🏻

@mgasner mgasner merged commit 03ae79f into master Apr 3, 2019
@mgasner mgasner deleted the kernel-fixture branch April 4, 2019 00:07
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.

3 participants