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

Prettier Stata error handling #22

Closed
hugetim opened this issue Oct 18, 2022 · 17 comments
Closed

Prettier Stata error handling #22

hugetim opened this issue Oct 18, 2022 · 17 comments

Comments

@hugetim
Copy link
Contributor

hugetim commented Oct 18, 2022

Currently, running list, nobs results in this error message:

Exception in thread Stata:
Traceback (most recent call last):
  File "C:\Users\tjhuegerich\AppData\Local\Continuum\anaconda3\envs\pystata-jupyter\lib\threading.py", line 1016, in _bootstrap_inner
    self.run()
  File "C:\Program Files\Stata17/utilities\pystata\core\stout.py", line 169, in run
    raise SystemError(output)
SystemError: option nobs not allowed
r(198);

Preferably, it would instead display the more Stata-like message:

option nobs not allowed
r(198);

Unfortunately, it doesn't appear possible to catch the SystemError raised by pystata in the way I expected. It is instead caught by the "threading.py" code referred to in the traceback. So I'm stumped on how to proceed, myself.

@hugetim
Copy link
Contributor Author

hugetim commented Oct 19, 2022

Unfortunately, it doesn't appear possible to catch the SystemError raised by pystata in the way I expected. It is instead caught by the "threading.py" code referred to in the traceback. So I'm stumped on how to proceed, myself.

I now think this is a flaw in the pystata code, so I am going to reach out to their technical support today to inquire about it.

@hugetim
Copy link
Contributor Author

hugetim commented Oct 20, 2022

I now think this is a flaw in the pystata code, so I am going to reach out to their technical support today to inquire about it.

Here's what I heard back:

Thank you for bringing this issue to our attention. Our developer said this was a bug. When you ran

stata.run("bad_command")

it indeed threw a SystemError exception as you can see from the following
output:

In [8]: try:
...: stata.run("bad_command")
...: except:
...: print("excepted")
...:
Exception in thread Stata:
Traceback (most recent call last):
File "C:\Users\tjhuegerich\AppData\Local\Continuum\anaconda3\lib\threadin=
g.py", line 926, in _bootstrap_inner
self.run()
File "C:\Program Files\Stata17/utilities\pystata\core\stout.py", line 169= , in run
raise SystemError(output)
SystemError: command bad_command is unrecognized r(199);

The reason you did not catch it is because this exception is thrown by a thread named Stata (see Exception in thread Stata above) instead of the main thread. If the exception is thrown by the main thread, there should not a line "Exception in thread Stata" above the traceback.

Our developers are working on a solution to fix this problem so that you can setup a customized try/catch. However, I am not sure at this moment when the fix will become officially available.

@hugetim
Copy link
Contributor Author

hugetim commented Nov 15, 2022

The underlying pystata flaw was fixed in today's Stata update, so it should be possible to handle errors in Stata code more gracefully now.

@hugetim
Copy link
Contributor Author

hugetim commented Nov 21, 2022

Unfortunately, the Stata/pystata update seems to have made it so that pystata-kernel no longer shows error output at all. Cells that trigger Stata errors are now basically skipped, other than being marked with an asterisk (in place of an execution count number).

My updated version of this kernel (with prettier error handling that works on the latest Stata update) is here: https://github.com/hugetim/nbstata

@ticoneva
Copy link
Owner

My apology for being absent for so long. Sickness and work have kept me from coming online.

Let me see if I understand the situation correctly:

  • If user updates Stata 17 to the latest version, pystata throws error in a different way, so that pystata-kernel is unable to display it.
  • The fact that only an asterisk is displayed means the kernel never returns.
  • The issue is fixed in your version?

@hugetim
Copy link
Contributor Author

hugetim commented Nov 29, 2022

I'm glad that you're better and back at it. 🙂

  • Right, pystata used to catch the error and not re-raise it, instead printing the traceback itself. Now pystata actually raises the error, and the kernel just stops running without reporting the error to Jupyter. (The IPythonKernel error-handling in the do_execute method has been overridden.)
  • Well, the cell with the error is marked with an asterisk but other cells continue running.
  • Yes, see the error handling in the do_execute method at the bottom here.

@roblem
Copy link
Contributor

roblem commented Jan 1, 2023

I'm going to be rolling out pystata-kernel (or some variant) for this upcoming Spring semester for a class I'm teaching beginning in three weeks. I've been following this issue as it seems super important for students who are learning stata to have error messages.

I'm wondering what the status is here versus the fork that seems to fix it by @hugetim. Are these changes going to be merged back into to pystata-kernel or is nbstata the "new" working version?

@hugetim
Copy link
Contributor Author

hugetim commented Jan 1, 2023

Are these changes going to be merged back into to pystata-kernel or is nbstata the "new" working version?

I think of nbstata as the new working version, myself, in short. I think the only potentially "breaking" change to be aware of is that I changed the default to echo 'None'. Let me know if any further changes would benefit your use case.

@ticoneva
Copy link
Owner

ticoneva commented Jan 2, 2023

My department's computing cluster has not yet been updated to the newest version of Stata---we only update between semesters---so it will take me some time to incorporate the changes back from @hugetim's fork. We are upgrading this week, then I will get to test it out. I agree that if this issue is important right now, use nbstata. I have added a note on this on the front page.

@roblem
Copy link
Contributor

roblem commented Jan 3, 2023

Thanks. Is it documented which stata update level pystata-kernel supports? I ask because as far as I know stata doesn't allow for incremental updates (a specific point/date release for Stata 17), you either stay with your current version or jump to the latest version (I could be wrong about this, but that is what I gather from help update and update query). On one of my machines I get this update possibility:

. update query
(contacting http://www.stata.com)

Update status
    Last check for updates:  03 Jan 2023
    New update available:    15 Nov 2022  (what's new)
    Current update level:    28 Jul 2022  (what's new)

and on another machine I get this:

  update query
(contacting http://www.stata.com)

Update status
    Last check for updates:  03 Jan 2023
    New update available:    15 Nov 2022
    Current update level:    23 Aug 2022

Note that on both I can only go to the 15 November version, I can't incrementally upgrade the first one to 23 August. Again, I could be wrong about this, but I haven't been able to find a way to do it.

In the future are you planning on keeping pystata-kernel up to date with your dept's current version of stata only? Because that could be problematic for other users who can't install that particular version due to the limitations of stata's update process.

@hugetim I suppose that nbstata will try to target the latest stata version (of course with lag times to iron out bugs).

@hugetim
Copy link
Contributor Author

hugetim commented Jan 3, 2023

@hugetim I suppose that nbstata will try to target the latest stata version (of course with lag times to iron out bugs).

I'd say that's up for discussion. Ideally it would be compatible with all builds--or else maintain separate versions that those stuck on a non-compatible Stata build could pin to. (My company doesn't always purchase the latest license, so there's a chance I won't be able to test it with Stata 18, myself, whenever that comes out. But then maybe Stata 18 will come packaged with its own kernel.)

At this point, I've only tested nbstata with the 15 Nov 2022 Stata update. So I'm actually not sure what the error behavior is for older Stata 17 builds--but my best guess is that it's the same as pystata-kernel.

@roblem
Copy link
Contributor

roblem commented Jan 3, 2023

Sorry. Didn't mean to be presumptuous. I am in a similar situation to @ticoneva: my cluster/jupyterhub manager decides when to install/upgrade stata and I am trying to map out the options to make things as easy as possible for them as they don't like to deal with stata even during the best of times (no technical glitches).

I should be able to test either pystata-kernel or nbstata against the latest version as our campus license keeps us current.

@hugetim
Copy link
Contributor Author

hugetim commented Jan 3, 2023

Didn't mean to be presumptuous.

Oh, nothing presumptuous came across to me. And that all makes sense. I appreciate you raising this potential issue to my attention.

@hugetim
Copy link
Contributor Author

hugetim commented Jan 5, 2023

Though it is out of date now, I thought to re-up this comment posted in another place, realizing that no one may have seen it, since it was a comment on a closed pull request: #21 (comment)

Oops, and for myself, I only just now noticed the recent commit which refers people to nbstata, which is much appreciated (and I recognize now is probably what occasioned these recent comments).

@ticoneva
Copy link
Owner

ticoneva commented Jan 9, 2023

Stata's inability to target specify update level is quite unfortunate. I do have access to snapshots of Stata 17 on our computing cluster, so it is possible to test packages against various Stata 17 versions if there is a need.

@hugetim
Copy link
Contributor Author

hugetim commented Jan 13, 2023

Nothing in the Jan 10 2023 Stata update appears relevant, but I did rerun my nbstata tests just to make sure and no issues came up.

@ticoneva
Copy link
Owner

I have ported the new error handling mechanism from nbstata. The code base has diverted significantly, however, so users who prefer new features and timely updates should still consider nbstata.

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

No branches or pull requests

3 participants