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

PR: "Profile by line" Button Behavior #30

Merged
merged 14 commits into from
Nov 26, 2018
Merged

PR: "Profile by line" Button Behavior #30

merged 14 commits into from
Nov 26, 2018

Conversation

cdfredrick
Copy link
Contributor

I had an issue where the green arrow "Profile by line" button in the widget would not work. The error turned out to be due to a Boolean value that was being passed by Qt (see Qt's docs) into the call of the profiling function as the working directory ("wdir" being the first keyword argument). To bypass this I wrapped the call to the profiling function in a lambda function that absorbs the extra argument.

Now that I could run line profiler from the GUI I noticed that the call from the "run" menu (or shift+f10) was not the same as the call from the widget GUI. I pulled the differences in from the main lineprofiler.py script into the widget's script, so things like the working directory from the run configuration and spyder's custom PYTHONPATH are now automatically included if started from the "run" menu or directly from the widget. A small change needed to get the run config imports working was normalizing the file paths with osp.abspath. I also made it so that running the profiler does not add the file path to its QComboBox if it is already there.

Wrapped the call to "start" in a lambda function. The optional argument captures the bool passed by the ".clicked.connect" method of the QToolButton.
Line profiler behavior is the same if started from the run menu or the widget gui. Run configuration is loaded in the widget. File names are normalized with osp.abspath for parity with file name strings from the editor. File names are only added once to the widget's QComboBox. Additional checks are made to properly select the working directory when automatically importing from the run configuration. Spyder's PYTHONPATH is automatically imported from the MainWindow instance passed to the widget.
A small change for 2.7 compatibility
@ccordoba12 ccordoba12 changed the title "Profile by line" Button Behavior PR: "Profile by line" Button Behavior Sep 23, 2018
@ccordoba12
Copy link
Member

@cdfredrick, thanks a lot for your help! It seems some of our continuous integration services are broken. I'm going to fix them and then I'll come back to you again.

@ccordoba12 ccordoba12 added this to the v0.1.2 milestone Sep 23, 2018
@ccordoba12
Copy link
Member

@cdfredrick, please merge or rebase with our latest master to fix the test errors.

Wrapped the call to "start" in a lambda function. The optional argument captures the bool passed by the ".clicked.connect" method of the QToolButton.
Line profiler behavior is the same if started from the run menu or the widget gui. Run configuration is loaded in the widget. File names are normalized with osp.abspath for parity with file name strings from the editor. File names are only added once to the widget's QComboBox. Additional checks are made to properly select the working directory when automatically importing from the run configuration. Spyder's PYTHONPATH is automatically imported from the MainWindow instance passed to the widget.
A small change for 2.7 compatibility
@jitseniesen
Copy link
Member

@cdfredrick Thanks for the PR! I am afraid that I haven't yet managed to look into it. The academic year is starting at my university and that is always a very busy time. Hopefully I'll find some time later this week to study the details and merge or give feedback. Feel free to nudge me if you haven't heard back after the weekend.

@jitseniesen
Copy link
Member

GitHub did something weird when the CI changes were merged in, because the commits seem to be duplicated, but that does not matter.

The first change (passing the bool) fixes a bug and is fine.

Regarding the second change (making the behaviour uniform), I think I agree that there is an issue that we want to fix but I want to make sure I understand it. Can you please check whether the Profiler in Spyder (the one you start with F10, not the line profiler) suffer from the same shortcoming?

As a side note, it would probably be good to factor out the common elements of the profiler plugins. I opened #32 to this purpose.

As a general design principle, we want to decouple the widgets (LineProfilerWidget) from the rest of Spyder and route all calls through the plugins (LineProfiler), so we are not supposed to store a reference to the spyder main window in the widget. See spyder-ide/spyder-unittest@7082b96 for how I did this in another plugin. We should think about how to handle the runconfig.

@jitseniesen
Copy link
Member

I forgot to add: Apologies for taking so long ...

Use signals to update the widget's copy of the Spyder PYTHONPATH
@cdfredrick
Copy link
Contributor Author

Using the unittest code as a guide, I have removed the reference to the main window in the widget and routed the python path updates through the plugin.

The regular Profiler has the same shortcoming. The "analyze" functions (that I updated in the second commit) in both the profiler and line_profiler are identical, outside of names referencing whether they belong to the profiler or line_profiler. The main issue that I am trying to fix is that starting the profiling process through the plugin, either through the run menu or through the keyboard shortcuts, grabbed the most recent python path and run configuration (or at least, tried to grab the run configuration), while starting it through the widget button did not.

@jitseniesen jitseniesen merged commit 86a11b9 into spyder-ide:master Nov 26, 2018
@jitseniesen
Copy link
Member

I am feeling a bit uncomfortable with using runconfig inside the widget, but it is not obvious to me how to improve what you did, so I merged your PR.

Thanks for your work and your patience.

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.

3 participants