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

Support for IPython %magics, like %timeit #28

Merged
merged 31 commits into from
Feb 27, 2022

Conversation

giladbarnea
Copy link
Contributor

Hi, thanks for pdbr!

Since IPython is already baked into RichPdb (if available), it itched me to unlock some IPython features, so I started with %magic commands.
This is something that I've wanted to do for some time now; pdbr stands for a lot of things I enjoy: terminal debuggers, IPython and rich library.

I've added 2 functions in _pdbr.py, wrote a test in test_pdbr.py, and added a description of the feature in the README.md.

I had trouble testing the actual magic functionality, because when trying to init a rich_pdb_klass(IPython.terminal.debugger.TerminalPdb) inside a test, it raises an exception, complaining about stdout not being a terminal. I couldn't get past that.

Additionally, some magics don't work out of the box, for example %lsmagic. This can be fixed by overriding onecmd(), but I wanted to know if you're at all interested in this PR's idea before adding more changes. Let me know if you want this fixed as well :)

Thanks,
Gilad

Copy link
Owner

@cansarigol cansarigol left a comment

Choose a reason for hiding this comment

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

Great job @giladbarnea . I was planning to work on it especially for %env command. With your PR all magic will be real :). Thanks for your effort.

Because of pytest --pdb is activated in project, you can see the real error via -s like pytest tests/test_pdbr.py -s. And you also need to put ipython package in test func of the noxfile.

pdbr/_pdbr.py Outdated
# This is indeed the case with do_pdef, do_pdoc etc,
# which are defined by our base class (IPython.core.debugger.Pdb).
result = getattr(self, f"do_{magic_name}")(line)
return result or ""
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return result or ""
return self._print(result) or ""

this change will fix some commands like lsmagic

pdbr/_pdbr.py Outdated
)
else:
result = magic_fn(arg)
return result or ""
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return result or ""
return self._print(result) or ""

@cansarigol
Copy link
Owner

@giladbarnea When i run a command and run magic secondly, i always see the last result with magic result. Can you have a look at it as well?

Screenshot from 2022-02-07 09-02-37

@giladbarnea
Copy link
Contributor Author

giladbarnea commented Feb 7, 2022

@cansarigol thanks for the quick reply :)
It looks like we're dealing with more than 1 issue, what do you think about writing tests for individual magics? I'd be happy to do it. There are a few dozen, I guess we'll find out we're not interested in some, or that others are working weirdly and require a more comprehensive fix.

About that bug with the repeating output, maybe it has to do with self.cmdqueue in pdb.Pdb.precmd(), or cmd.Cmd.emptyline(). I'll have to check that.

By the way, maybe it's worth sharing, I've made a pseudocode gist of the main loop. It helps me sort out the inheritance / method overload mess (cmd.Cmd > pdb.Pdb > IPython.core.debugger.Pdb > IPython.terminal.debugger.TerminalPdb > RichPdb):

class IPython.terminal.debugger.TerminalPdb:
	def cmdloop():
		self.preloop()
		# pdb.Pdb.preloop() displays expressions set by `display` command
		
		stop = None
		while not stop:
			line = self.cmdqueue.pop(0)
			line = self.precmd(line)		
			# core.debugger.Pdb.precmd() does line="pinfo "+... if ends with ?, 
			# then calls super which is pdb.Pdb.precmd()
			# pdb.Pdb.precmd() handles self.aliases and appends to self.cmdqueue
			
			stop = self.onecmd(line)
			# pdb.Pdb.onecmd() calls cmd.Cmd.onecmd(), 
			# unless we're executing `commands`.
				cmd.Cmd.onecmd(line):
					cmd, arg, line = self.parseline(line)
					if not line:
						return self.onecmd(self.lastcmd) if self.lastcmd else None
					# pdb.Pdb.default() compiles and execs the line
					if not cmd:
						return self.default(line)
					if self.do_{cmd}:
						return self.do_{cmd}(arg)
					return self.default(line)
			
			stop = self.postcmd(stop, line)
			# cmd.Cmd.postcmd() just returns stop as-is

		self.postloop()
		# cmd.Cmd.postloop(): pass

Are there any magics that you don't want altogether?

@cansarigol
Copy link
Owner

@giladbarnea writing tests for individual magics and making out TerminalPdb class and overriding its cmdloop are great ideas, please go ahead. My only objection would be for the %pdb but I don't have too much experience with magics.

@giladbarnea giladbarnea marked this pull request as draft February 7, 2022 15:23
@cansarigol
Copy link
Owner

Hi @giladbarnea your PR looks great 👍. If you don't have time to put inherited TerminalPdb class, let's merge it as is and you can pursue with a new PR. Thanks for your effort btw

@giladbarnea
Copy link
Contributor Author

giladbarnea commented Feb 26, 2022

Hi @giladbarnea your PR looks great +1. If you don't have time to put inherited TerminalPdb class, let's merge it as is and you can pursue with a new PR. Thanks for your effort btw

Hi @cansarigol, you're right, I couldn't find the time (had my first kid a week ago :))
Let me see what quick wins I can get us within the next 45 minutes, I'll prepare a good-enough commit and hopefully it'll be ok.

@giladbarnea giladbarnea marked this pull request as ready for review February 26, 2022 14:46
@giladbarnea
Copy link
Contributor Author

@cansarigol, I've branched out to run-magic-through-ipython to continue work there on making more magics work (by capturing self.shell.run_cell(...)'s output), so you can merge the commit I just made f4294af8 if you want.
Thanks!

@cansarigol
Copy link
Owner

Hi @giladbarnea your PR looks great +1. If you don't have time to put inherited TerminalPdb class, let's merge it as is and you can pursue with a new PR. Thanks for your effort btw

Hi @cansarigol, you're right, I couldn't find the time (had my first kid a week ago :)) Let me see what quick wins I can get us within the next 45 minutes, I'll prepare a good-enough commit and hopefully it'll be ok.

congratulations 👏 👶 🎉 . hope everybody is doing fine.

noxfile.py Outdated
"icecream",
"prompt_toolkit",
"sqlparse",
"IPython",
Copy link
Owner

Choose a reason for hiding this comment

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

For the latest version of Ipython, there is a bug that is about to get reverted.

Suggested change
"IPython",
"IPython==7.x",

@cansarigol
Copy link
Owner

@giladbarnea tests fail for me. Are they same for you as well?

@giladbarnea
Copy link
Contributor Author

@giladbarnea tests fail for me. Are they same for you as well?

@cansarigol Yes, I think at the very least the session.install() statement in noxfile.test() should include a . to install pdbr itself, and session.run() should include a "--capture=no" arg.
This fixes some tests but others are still failing, I'll update when it's fixed

…: install pdbr itself, IPython==7.32.0, and run pytest with --capture=no and --disable-warnings (same for setup.cfg)
@giladbarnea
Copy link
Contributor Author

@cansarigol That's it, f13020f succeeded through the CI pipeline.

@cansarigol cansarigol merged commit 4dbfe80 into cansarigol:master Feb 27, 2022
@cansarigol
Copy link
Owner

@giladbarnea Great, thanks for your effort again. I am looking forward to using it myself first 😺

we can fix windows test errors later.

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

2 participants