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

BufferStock REMARK do_all.py crashes loading IPython shell #19

Closed
sbenthall opened this issue Oct 28, 2019 · 10 comments
Closed

BufferStock REMARK do_all.py crashes loading IPython shell #19

sbenthall opened this issue Oct 28, 2019 · 10 comments
Assignees

Comments

@sbenthall
Copy link
Contributor

Running this do_all.py from the command line:
https://github.com/llorracc/BufferStockTheory/blob/5ccb22b72cb9fd46245b8954459ce5f44ab3c7ea/Code/Python/do_all.py

gets me the following error in the terminal:

Traceback (most recent call last):
  File "do_all.py", line 3, in <module>
    import BufferStockTheory
  File "/home/sb/projects/econ-ark/REMARK/REMARKs/BufferStockTheory/Code/Python/BufferStockTheory.py", line 154, in <module>
    get_ipython().run_line_magic('matplotlib', 'inline')
AttributeError: 'NoneType' object has no attribute 'run_line_magic'

This looks like it's because the get_ipython() method returns None when no InteractiveShell instance is registered:
https://ipython.readthedocs.io/en/stable/api/generated/IPython.core.getipython.html

(Please let me know if these issues are better tracked to the BufferStockTheory repo in llorracc.)

@project-bot project-bot bot added this to Needs Triage in Issues & PRs Oct 28, 2019
@MridulS
Copy link
Member

MridulS commented Oct 31, 2019

@sbenthall Are you running python do_all.py or ipython do_all.py? These scripts are supposed to work with ipython not the base python one.

@llorracc
Copy link
Collaborator

llorracc commented Oct 31, 2019 via email

@sbenthall
Copy link
Contributor Author

Ah, you're right. I probably was running it with python, to ipython.

I see now that in the comments at the start of the file, it says to use ipython.

This is maybe not the first place one would look. Maybe this note about using ipython could go in the README section about the do_all.py scripts.

https://github.com/econ-ark/REMARK#do_py

I think if it were documented, I wouldn't have made the mistake.

I think I would find the message proposed by @llorracc confusing because I didn't think the do_[].py scripts were notebooks. Is this do_[].py construct specific to REMARK as a coding pattern?

@llorracc
Copy link
Collaborator

llorracc commented Oct 31, 2019 via email

@sbenthall
Copy link
Contributor Author

Ok. I see now that there are a few interwoven issues here.

  • If many people are having this problem, the do_[].py scripts should have that code in them as you say.
  • this is presently difficult to enforce, because the REMARKs (including the BufferStockTheory on that started this thread) is a git submodule in a different organization. I just checked and it looks like there are a wide variety of formats and coding styles in do_[].py scripts.

I wonder if it would be possible to be more thoughtful about how these do_[].py scripts are designed, how that design is enforced when a new REMARK is included in this repository as a submodule, and whether any of the code necessary for complying with those design requirements could be in one place in the REMARK repository, rather than duplicated across every REMARK submodule.

@llorracc
Copy link
Collaborator

llorracc commented Nov 3, 2019

The solution for this is probably to add something in the first line of the Jupyter notebook that tests whether it is being run using ipython. When the [notebook].py jupytext file is autogenerated, if someone runs it using python [notebook].py they should get an error message.

@sbenthall
Copy link
Contributor Author

This code will suffice for warning the user if they run a file in python instead of ipython:

try:
    get_ipython()
except:
    print("This file should be run with the `ipython` command, not `python'.")

@llorracc
Copy link
Collaborator

llorracc commented Nov 7, 2019

I just started to implement this in the BufferStockTheory REMARK, but noticed that the setup cell already contains:

from IPython import get_ipython # In case it was run from python instead of ipython

which should already be sufficient. (It will presumably halt with an error if ipython is not present).

So, could you go back and look again to see if you can figure out what was causing your earlier problems? It sounds like you've fixed it now, but it couldn't have been only the fact of running from the command line...

@sbenthall
Copy link
Contributor Author

Ok, I'm looking into this again.

Currently, there are two do_all.py files. One is at the top level directory BufferStockTheory/ of that REMARK. The other is in BufferStockTheory/Code/Python.

Both of them now give me this error (I get a similar error whether running in IPython or Python):

$ ipython do_all.py 
---------------------------------------------------------------------------
ModuleNotFoundError                       Traceback (most recent call last)
~/projects/econ-ark/REMARK/REMARKs/BufferStockTheory/Code/Python/do_all.py in <module>
      1 # do_all.py is required for a REMARK; in this case, it just runs BufferStockTheory.py
      2 
----> 3 import BufferStockTheory
      4 
      5 

ModuleNotFoundError: No module named 'BufferStockTheory'

I believe this is because:

  • Both do_all.py's execute Code/Python/BufferStockTheory.py
  • There is no Code/Python/BufferStockTheory.py file
  • There is a notebook committed to the repository, Code/Python/BufferStockTheory.ipynb

I'd recommend either:

  1. Replacing the .ipynb file with a .py file in the repository, or
  2. Adding a line to the do_all.py that runs the jupytext command to create the .py file
  3. Consolidate down to just one do_all.py file

My preferences is for (1) over (2). I think (3) would be good in any case.

@llorracc
Copy link
Collaborator

llorracc commented Mar 9, 2020

Closing because issues above are (mostly) resolved.

@llorracc llorracc closed this as completed Mar 9, 2020
Issues & PRs automation moved this from Needs Triage to Done Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues & PRs
  
Done
Development

No branches or pull requests

3 participants