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

Exceptions raised when executing notebooks via the %run magic command #12301

Merged
merged 4 commits into from May 31, 2020

Conversation

palewire
Copy link
Contributor

@palewire palewire commented May 9, 2020

A first step toward fixing #12291. I only addressed the execution of ipynb files because I lacked the confidence to edit the complex shell operations below.

It's unclear to me if you would want the ultimate solution to be this narrowly tailored, or if you'd prefer other elements of this function to change behavior as well. If you let me know what you'd like, I can attempt to implement it.

I can verify that this modest patch did in fact work in my notebook testing.

@choldgraf
Copy link

choldgraf commented May 11, 2020

This seems good to me! Do you think we could add a test to make sure that this will properly fail when run on a notebook w/ a %run that doesn't work? (though I am not super familiar with ipython's testing infra...maybe @MSeal has a better idea than I?)

Copy link
Contributor

@MSeal MSeal left a comment

I think you need to change all the places with a return above the line you edited to instead raise the given exception (or create one). Otherwise typos or missing files will error silently and not raise as well.

@MSeal
Copy link
Contributor

MSeal commented May 11, 2020

This seems good to me! Do you think we could add a test to make sure that this will properly fail when run on a notebook w/ a %run that doesn't work? (though I am not super familiar with ipython's testing infra...maybe @MSeal has a better idea than I?)

Doesn't have to be notebook specific. A test could be to call the run function with an execution target that will always fail and make sure it raises an exception. To be thorough, have a test for each failure path to ensure if raises an exception.

@palewire
Copy link
Contributor Author

palewire commented May 11, 2020

@MSeal I've added some additional exceptions. Is that what you were after?

@MSeal
Copy link
Contributor

MSeal commented May 11, 2020

@palewire Yep. I think you missed one more:

@palewire
Copy link
Contributor Author

palewire commented May 11, 2020

Okay @MSeal, I've added that one yet too. Anything else I can do?

@MSeal
Copy link
Contributor

MSeal commented May 11, 2020

Probably to make this mergable, we need some updated or new tests in https://github.com/ipython/ipython/blob/master/IPython/core/tests/test_run.py.

Thanks for contributing and getting this fixed btw :)

@MSeal
Copy link
Contributor

MSeal commented May 11, 2020

@Carreau Can comment if there's anything else since he's the maintainer here

@palewire
Copy link
Contributor Author

palewire commented May 12, 2020

I've pushed a fresh commit with some unit tests. Take a look and let me know what else I can do.

@choldgraf
Copy link

choldgraf commented May 12, 2020

Nice! It looks good to me!

MSeal
MSeal approved these changes May 12, 2020
Copy link
Contributor

@MSeal MSeal left a comment

LGTM

@palewire
Copy link
Contributor Author

palewire commented May 12, 2020

Thanks, guys. @Carreau, let me know if there's anything else I can do.

@Carreau Carreau added this to the 7.16 milestone May 31, 2020
@Carreau Carreau merged commit c24e8af into ipython:master May 31, 2020
2 checks passed
@meeseeksdev
Copy link
Contributor

meeseeksdev bot commented May 31, 2020

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 7.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 c24e8af4bf631b51ddec077d13022aba00dcf30e
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #12301: Exceptions raised when executing notebooks via the %run magic command'
  1. Push to a named branch :
git push YOURFORK 7.x:auto-backport-of-pr-12301-on-7.x
  1. Create a PR against branch 7.x, I would have named this PR:

"Backport PR #12301 on branch 7.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@meeseeksdev meeseeksdev bot added the Still Needs Manual Backport label May 31, 2020
@Carreau
Copy link
Member

Carreau commented May 31, 2020

Thanks, looks good, sorry this ended up hidden with all the open PRs.

Carreau added a commit to Carreau/ipython that referenced this issue May 31, 2020
Carreau added a commit to Carreau/ipython that referenced this issue May 31, 2020
Carreau added a commit that referenced this issue Jun 1, 2020
Backport PR #12301: Exceptions raised when executing notebooks via the e %run magic command
@Carreau Carreau removed the Still Needs Manual Backport label Jun 26, 2020
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.

None yet

4 participants