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

Extension to mpi programs #351

Open
wants to merge 108 commits into
base: master
Choose a base branch
from

Conversation

incardon
Copy link

@incardon incardon commented Aug 24, 2020

  • [] I have added an entry to docs/changelog.md

Summary of changes

This diff add support to work with mpi programs in Gdbgui.

The main changes regard adding the mpi_rank element to debug session a way to search the gdbsession by rank extend run_gdb_command with the process id that is default to -1. -1 in MPI based debug session -1 mean run the command on all sessions. in case of non-MPI is ingnored and -1 mean run on the single underline gdb session. An additional HTML page on the server to give to the client access to the MPI servers informations. Process information is attached to all gdb response answer in case of non MPI process information is -1. process information has been also attached to some function in Actions now process information of the message process, but because it is a defaulted to -1 so should not produce problem if there is code around calling the function in the old form.

inferior_program_paused has been changed to process processor information

change_process_on_focus: and refresh_state_for_gdb_pause has been added to process change of processor on focus and update of the GUI information

connect_to_gdbserver_mpi is added to handle the connection to MPI gdb server

Several global variables to initial_store_data has been added to store information for each processor

A processo status bar has been added when MPI programs has been debugged

gdbgui/src/js/Breakpoints.tsx has been fixed to remove breakpoint placed in templated code

All around there are change to handle process information, when needed

Test plan

2 tests has been added one for the JSX side that test server + Gdbgui TypeScript code using puppeteer and chrome headless, emulating click events and keyboard typing. The test go trough a full debug session.

The second test test is for the python server the test send command directly to the server to go through a debug session.
Tested by running

# command(s) to exercise these changes

The best is to look at the

.github/workflows/tests.yml

for the required machine setup.

The rest is:

launching an mpi program with the script ./gdbgui-mpi/launch_mpi_debugger 6 ./my_program -arg1 -arg2

where 6 is the number of process. Launch gdbgui, select connect to MPI gdbservers use *:6000 as input test for the server

@incardon
Copy link
Author

incardon commented Aug 24, 2020

I know is a big commit. I will first start to pull from the actual master. But would be nice to have a point of discussion in case there is interest to merge in the actual gdbgui.

@incardon
Copy link
Author

ok I started to merge and I found the first problem when I try to use the server ...

File ".../gdbgui/gdbgui/server/sessionmanager.py", line 9, in
from pygdbmi.IoManager import IoManager
ModuleNotFoundError: No module named 'pygdbmi.IoManager'

Where do I get IoManager ? I do not see in the repo of pygdbmi

@incardon
Copy link
Author

incardon commented Aug 24, 2020

It is in add_reader branch ... of pygdbmi ... and is failing in your CI

Also I am not able to make this last refactor working:

The server die with:

File "/home/i-bird/Desktop/MOSAIC/OpenFPM_project/gdbgui/gdbgui/cli.py", line 254, in main
gdbgui.server.server.run_server(
AttributeError: module 'gdbgui.server' has no attribute 'server'

Not sure how it should works.

OK fixed with import gdbgui.server.server in line 25 of cli.py

Comment on lines 208 to 215
jQuery.ajax({
url: "mpi_processes_info",
success: function(data, status) {
data_lines = data.split(/\r?\n/);
},
async: false
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

I am going to eventually replace all jQuery.ajax calls with fetch.

It would be helpful if this was switched to use fetch, and the function turned into an async function. If you don't know what that means, don't worry about it. I will just fix it in a future update.

Copy link
Author

Choose a reason for hiding this comment

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

I will have a look. My programming skill in python and Javascript ( now TypeScript ) are basic, I am learning while going.

Copy link
Owner

Choose a reason for hiding this comment

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

It’s fine to leave as is. I can migrate it along with the other calls at a later time.

* @param state mpi state
* @return nothing
*/
set_mpi_state: function(state) {
Copy link
Owner

@cs01 cs01 Aug 25, 2020

Choose a reason for hiding this comment

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

All files are now typescript, so arguments like state can be annotate with types now. I will be working to update existing code, but would like to see new code annotated from the start.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I saw merging with master ... I am already changing all of them doing it.

@cs01
Copy link
Owner

cs01 commented Aug 25, 2020

Hi, thanks for the PR! Very impressive.

I just landed a whole bunch of changes to gdbgui recently. Unfortunately those changes have caused conflicts with this PR. Most of the code that was changed was on the backend. The frontend code was not affected very much other than the switch from .js/.jsx to .ts/.tsx. I just merged the pygdbmi add-reader branch into master and published a new version of pygdbmi (0.10.0.0) which is required by the latest version of gdbgui (0.14.0.0).

I looked at your screenshot in the related issue. The only thing I have to suggest is to move the processor selection to the right pane.

I am looking forward to seeing the merge conflicts resolved and giving it a try. I am curious -- is there a particular project or company these changes already being used in, or did you make this just in case you will need it in the future?

@@ -0,0 +1,15 @@
from gdbgui import backend
Copy link
Owner

Choose a reason for hiding this comment

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

is this new file needed or can the existing cli.py be used?

Copy link
Author

@incardon incardon Aug 25, 2020

Choose a reason for hiding this comment

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

Probably is removable, I will have a look

@@ -0,0 +1,3 @@
#! /bin/bash
Copy link
Owner

Choose a reason for hiding this comment

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

I think the gdbgui-mpi folder might fit better in examples/mpi. There are already examples in the examples folder.

store.connectComponentState(this, ["processors_states"]);
}
render() {
let btn_class = "btn btn-default btn-sm";
Copy link
Owner

Choose a reason for hiding this comment

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

Bootstrap classes are being phased out in favor of tailwind css.

Copy link
Owner

Choose a reason for hiding this comment

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

If you don’t know tailwind that’s is okay. I will update it all later.

@@ -165,8 +169,10 @@ class SourceCode extends React.Component {
) {
let row_class = ["srccode"];

if (is_gdb_paused_on_this_line) {
if (is_gdb_paused_on_this_line === 1) {
Copy link
Owner

Choose a reason for hiding this comment

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

This is a boolean, it doesn't make sense to compare to a number. If the semantics around this have changed, the variable name and type should change.

noxfile.py Outdated
@@ -13,7 +13,7 @@

@nox.session(reuse_venv=True, python=python)
def tests(session):
session.install(".", "pytest", "pytest-cov")
session.install(".", "pytest", "pytest-cov", "flask-socketio>4.2.0")
Copy link
Owner

Choose a reason for hiding this comment

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

"flask-socketio should already be included (if necessary) with the . target

Copy link
Author

@incardon incardon Aug 25, 2020

Choose a reason for hiding this comment

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

I will check again the problem of not having the > 4.2 it was that it was installing the version 2.9.3 (I think). And I needed flask-socketio bigger than 3.3 because of test_backend_mpi.py. I introduced a test that to emulate a debug session for the server and I had to connect the HTTP cookies of the flask client to the flask-io socket. This feature was introduced in flask-socketio 3.3

Copy link
Owner

@cs01 cs01 Aug 25, 2020

Choose a reason for hiding this comment

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

If the version is upgraded (and everything still works) that is fine, but it should be done in setup.py, which is the where Gdbgui defines its dependencies.

@incardon
Copy link
Author

incardon commented Aug 25, 2020

We are a research group specialized in Parallel simulations and I am one of the software developer in HPC. From long time I was searching for a small project that could the adapted to become an opensource parallel debugger for the HPC community. And this fullfill all the requirements. In few days was possible to create something working

@cs01
Copy link
Owner

cs01 commented Aug 25, 2020

We are a research group specialized in Parallel simulations and I am one of the software developer in HPC. From long time I was searching for a small project that could the adapted to become an opensource parallel debugger for the HPC community. And this fullfill all the requirements. In few days was possible to create something working

Do you have a website I could check out?

@incardon
Copy link
Author

incardon commented Aug 25, 2020

http://mosaic.mpi-cbg.de/

While for the simulation library

http://openfpm.mpi-cbg.de

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.

2 participants