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

feat: launch by kernelspec #1264

Merged
merged 9 commits into from
Jan 12, 2017
Merged

feat: launch by kernelspec #1264

merged 9 commits into from
Jan 12, 2017

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Dec 5, 2016

Beginning efforts to make it so we can launch by kernelspec rather than
kernelspec name. This is to enable conda environments, an embedded JS kernel,
and make it easier to do remote kernels by refactoring how we launch.

  • Launch new notebook from command line
  • Launch new notebook from menu
  • Change kernel
  • Open old notebook
  • More test coverage
  • Ensure notebook schema conformance

@codecov-io
Copy link

codecov-io commented Dec 5, 2016

Current coverage is 91.76% (diff: 89.65%)

Merging #1264 into master will decrease coverage by 0.06%

@@             master      #1264   diff @@
==========================================
  Files            57         57          
  Lines          1482       1494    +12   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1361       1371    +10   
- Misses          121        123     +2   
  Partials          0          0          

Powered by Codecov. Last update f10a5ed...792f20d

@rgbkrk rgbkrk changed the title feat: launch by kernelspec [WIP] feat: launch by kernelspec Dec 5, 2016
@rgbkrk
Copy link
Member Author

rgbkrk commented Dec 5, 2016

This PR is excellent proof of missing tests for main. 😉

@alexandercbooth
Copy link
Member

I'd be happy to start chipping away on some tests for main, if someone wouldn't mind giving me a little guidance on where to start.

@rgbkrk
Copy link
Member Author

rgbkrk commented Dec 5, 2016

I'd be happy to start chipping away on some tests for main, if someone wouldn't mind giving me a little guidance on where to start.

I'm going to break so many things right now that I might as well make a testing harness for some of what's in main here.

The main thing is that much of main can be tested just like the notebook. Try to find functions that don't require the filesystem or electron APIs (there are a few I know of) and you'll be off to a start.

@jdetle
Copy link
Member

jdetle commented Dec 5, 2016

The main thing is that

I c wut u did thar

@rgbkrk
Copy link
Member Author

rgbkrk commented Dec 11, 2016

Ok this is now "ready". By that I mean that it's ready for me to update tests now that I have it working.

@rgbkrk rgbkrk changed the title [WIP] feat: launch by kernelspec feat: launch by kernelspec Dec 11, 2016
@rgbkrk rgbkrk changed the title feat: launch by kernelspec [WIP] feat: launch by kernelspec Dec 11, 2016
@rgbkrk
Copy link
Member Author

rgbkrk commented Dec 12, 2016

This still fails conformance tests, I'll come back to it later.

Beginning efforts to make it so we can launch by kernelspec rather than
kernelspec name. This is to enable conda environments, an embedded JS kernel,
and make it easier to do remote kernels by refactoring how we launch.
@rgbkrk
Copy link
Member Author

rgbkrk commented Jan 12, 2017

Rebased, going to see what likely lint errors exist now.

@rgbkrk
Copy link
Member Author

rgbkrk commented Jan 12, 2017

Sure enough, this PR still has an issue with conformance:

$ npm run test:conformance

> nteract@0.0.15 test:conformance /Users/kylek/code/src/github.com/nteract/nteract
> node scripts/conformance.js

✅	/Users/kylek/code/src/github.com/nteract/nteract/example-notebooks/altair.ipynb
❌	/Users/kylek/code/src/github.com/nteract/nteract/example-notebooks/blah.ipynb
[ ValidationError {
    property: 'instance.metadata.kernelspec',
    message: 'requires property "display_name"',
    schema:
     { description: 'Kernel information.',
       type: 'object',
       required: [Object],
       properties: [Object] },
    instance: { name: 'python3' },
    name: 'required',
    argument: 'display_name',
    stack: 'instance.metadata.kernelspec requires property "display_name"' } ]

I'll see if I can iron that out. Otherwise, kernels are executing just fine.

@rgbkrk rgbkrk changed the title [WIP] feat: launch by kernelspec feat: launch by kernelspec Jan 12, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Jan 12, 2017

Finally finished off this branch -- the kernel info is set properly in the document now. It's very ready for review. 😄

@jdetle jdetle merged commit 51ff540 into nteract:master Jan 12, 2017
Copy link
Member

@jdetle jdetle left a comment

Choose a reason for hiding this comment

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

Per advice in redux-observable/redux-observable#108,

ActionsObservable.of(executeCell('id', 'source')));
  executeCellEpic(action$, store)
    .toArray()
    .subscribe(actions => {
      expect(actions).to.deep.equal([
        kernelNotConnected() // whatever action creators
      ]);
      done();
    });
});

is a more succinct pattern to follow in the future.

@rgbkrk rgbkrk deleted the launch-kernelspecs branch January 12, 2017 20:37
@lock
Copy link

lock bot commented Apr 3, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants