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

RRPlugins #670

Merged
merged 29 commits into from
Oct 5, 2020
Merged

RRPlugins #670

merged 29 commits into from
Oct 5, 2020

Conversation

debashish05
Copy link

This pull request is done under GSoC 2020. Some major points are as follows:

  1. This provides rrplugins to build independently of roadrunner. Now change in roadrunner doesn't need to rebuild rrplugins and vice versa.
  2. Add Noise and testModel plugin is ported to this new system.
  3. rrVersionsInfo.h file is needed by rrplugins. Earlier roadrunner needs to be installed first then rrplugins need this in the linking stage. But now since rrplugins and roadrunner are the same builds this file needs to present in the source code.
  4. Currently, there are folders in rrplugins that are not working that need to be cleaned by me, Eg. rrplguins/Examples.
  5. All the plugins are needed to be ported to this new architecture and I am working on it.
  6. Currently, by default, this code will not affect roadrunner, and will not included in roadrunner by default. But can be used by turning RRPlugins support while the Cmake step.
  7. Modifed the CPython wrapper to make it work in Python 3 for rrplugins.

@kirichoi
Copy link

Apart from build issues, there are few other things that needs to be done before merging:

  1. Wrapper integration to roadrunner build system
  2. Cleaning the source to the rrplugins core

@debashish05
Copy link
Author

The test model plugin is not working in Linux, because loadSBML is not able to load the XML file. Need to look at the issue. Also, after execution python program, there is an error message "free(): invalid pointer Aborted (core dumped)". This also needs to be fixed.

For points (1) Integration of both wrapper will a bit tough mainly because of debugging issues in SWIG. For the time being, I am sticking with CPython wrapper and keep the integration of wrapper as the long term goal.
(2) Yes, surely need to look at this issue.

@debashish05 debashish05 marked this pull request as draft August 29, 2020 10:21
@debashish05 debashish05 marked this pull request as ready for review October 1, 2020 22:20
@luciansmith
Copy link

I looked at all the bits that aren't in the rrplugins/ directory, and there are only a couple changes I would make:

  • The rrVersionInfo.h file should not be included: it's created from the rrVersionInfo.h.in file, instead.
  • The rrRoadRunner.cpp differences seem to be entirely 'deleted blank lines' and other whitespace changes. Those can probably be dropped, but it's not a huge deal either way.

I can make the changes myself if you want. Thank you!

@debashish05
Copy link
Author

debashish05 commented Oct 3, 2020

  1. RRPlugins need that header file which is autogenerated at the compile time. Earlier when roadrunner is installed first the rrVersion.h file gets generated first and then we go on building rrplugins. So at compile time rrplugins used to get the rrVersion.h file. But since we are merged two separate processes into one, now at compile time there will be no rrVersion.h for rrplugins. So, it will show error can't open the include file.
    RRPlugins need rrVersion.h because it is using the roadrunner c wrapper file. That's why included that file as a temporary fix.

Fix: Modified the CMakeLists to make it a proper fix. Now we don't need to include the rrVersionInfo.h.

  1. For the second point, I experimented with the main roadrunner as well early on so maybe these changes are because of that. It will be very kind if you can check the blank lines thing. Any more recommendations or updates you want in the code. I have also updated the binaries in the google drive link that I have shared. Thanks for sharing your view regarding the code.

Debashish Roy and others added 2 commits October 3, 2020 19:59
I also found this problem in my own branch--logging to debug is OK, but logging to stderr, as you noted, is not.  This should make your and my changes compatible.
@debashish05
Copy link
Author

Thank you for fixing this. Anything else from my side, I need to fix it?

@luciansmith luciansmith merged commit 3262490 into sys-bio:develop Oct 5, 2020
@luciansmith
Copy link

Looks good! Thank you so much!

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

3 participants