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.

Sign up for GitHub

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

Jump to bottom

Dependency on dvs_msgs #4

Open
weidel-p opened this issue Feb 21, 2017 · 26 comments
Open

Dependency on dvs_msgs #4

weidel-p opened this issue Feb 21, 2017 · 26 comments

Comments

@weidel-p
Copy link
Owner

The whole toolchain is now dependent on 'dvs_msgs' which is unnecessary for most applications. One solution would be to separate the toolchain into modules like @mhoff suggested in #1.
@mhoff
Copy link
Contributor

mhoff commented Feb 21, 2017 •
edited
Loading

As discussed in my original proposal, catkin does not support optional dependencies. As a direct consequence, something like a dvs_adapter can not be provided in the same package as the general adapters. Thus, it seems to be reasonable to ship the general adapters in some kind of "core" package and to provide more specialized adapters (with additional dependencies) in separate catkin packages.

The question is, what is the most clean approach to this modularization in relation to Git/GitHub. Off the top of my head, I see these possibilities:

  1. Git submodules as proposed in the original comment. Obvious advantage of this approach would be to provide all possible adapters in one repository and the user is able to activate the specialized adapter projects he wants to use. Disadvantage: Maintenance becomes more difficult. Every update in a specialized projects has to be reflected in the core project. This is a unnatural connection, especially because catkin packages provide some kind of versioning themselves.
  2. Every catkin package becomes a Git repository on its own. These repositories could then be grouped in a GitHub organization for this purpose and for the sake of point (1) we could even provide a meta repository containing all adapter projects as submodules. With some hacks Git allows submodules to track not single commits, but upstream branches. This approach could be used to provide a Makefile in the meta repository which simply updates all submodule to the newest version.
  3. Everything is part of one big repository and some kind of build system allows for activating/deactivating certain parts. This would be some kind of a hack, because the catkin package meta configuration would have to be generated based on the users preferences. I dislike this approach, but maybe it should be part of this discussion.

For options (1) and (2), it is necessary to let the 'core package' export some central include files, which should provide all the basic ROS-MUSIC stuff. In essence, specialized adapters should focus on translating the MUSIC buffer to the ROS world and the other way around. Update rates, music timestep, ROS threads and all this technical stuff should be handled in the parent class as good as possible.

At the moment, most of the adapters are very redundant in their implementation. Even if it won't come to realizing the above approach, a refactorization should take place at some point.

Personally, I would prefer option (2) at the moment. It appears to be the most clean solution to me.

@mhoff
Copy link
Contributor

mhoff commented Mar 25, 2017

Maybe there is another, more convenient, solution.

With mhoff/ros_music_adapter@d7bdf7c I split the catkin package into two catkin packages, both residing in the same repository. The core subfolder contains the "classic" ros-music toolchain and the dvs subfolder contains my dvs-specific adapter. This can be seen as a first step towards (1) and (2), but as a compromise, one could also exploit the sparse checkout feature of git.

Example

git clone -b modules --no-checkout [email protected]:mhoff/ros_music_adapter.git
cd ros_music_adapter
git config core.sparseCheckout true
echo 'core' > .git/info/sparse-checkout
git checkout

$ ls
core

Spares checkouts allow the users to select the components they want to use explicitly. I think this might be an interesting (and especially quick) solution for our dependency problem.

@mdjurfeldt
Copy link
Collaborator

@mhoff Interesting!

@weidel-p and I had a discussion about how to go forward. We arrived at the idea to split into a core part without much ROS dependencies, which could be part of the MUSIC distribution, and a ROS-dependent part.

Didn't you two talk about this? Philipp?

@mhoff
Copy link
Contributor

mhoff commented Mar 25, 2017

@mdjurfeldt What do you mean by "without much ROS dependencies"? Do you think of the whole "classic" toolchain, or do you think of significantly more general code which could also be used for adapting to other communication frameworks (e.g. ZMQ)?

In my opinion, it is most profound to keep all the ROS-MUSIC-related code as close as possible in terms of file system, build system and coding standard.

@weidel-p @mdjurfeldt Did you already define some kind of protocol for ZMQ + SNN? I just began a few weeks ago to use ZMQ to keep data persistence and plotting out of the loop, so I would be interested in your insights. It does not really make sense for me to develop a completely custom protocol for transportation of spiking data when another group does the same.

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Mar 25, 2017 via email

@mhoff
Copy link
Contributor

mhoff commented Mar 26, 2017

@mdjurfeldt https://github.com/mdjurfeldt What do you mean by "without much ROS dependencies"? Do you think of the whole "classic" toolchain, or do you think of significantly more general code which could also be used for adapting to other communication frameworks (e.g. ZMQ)?

In Philipp's ROS-MUSIC toolchain, there is, for example, decoders and encoders which are entirely ROS-independent. Those things could be part of the MUSIC distribution.

True. These are indeed components which should be 'natively' provided by MUSIC. As a sidenote: The decoder reads the weight matrix from a JSON-file. This makes the usage rather clumsy. Would it be possible to transport this matrix as a MUSIC-parameter? I can imagine that big matrices might become a problem as a command-line argument.

Regarding the splitting into two catkin packages which you discuss now, where do you draw the line between "core" and the rest?

First of all, the "core" package provides common data, like shared cmake-files, shared libraries (rtclock) and shared abstract functionality for writing adapters. The idea is that the "dvs" package simply has to subclass an existing abstract adapter, write a custom setup + ROS callback and that's it (everything else is largely redundant for most adapters).
I think allowing for quick creation of new adapters using this method is crucially important for people using ROS-MUSIC.

As for the provided components, the minimum is to draw the line based on dependencies, leading to "core" being a collection of components depending purely on the standard libraries/packages (ROS, MUSIC, pthread, jsoncpp, ...) and everything else being in separate packages.
Further, it should be clear that domain-specific components can (should?) be placed in other packages.

But I do not think that we have to define a strict set of rules as of now. Using pull requests we can easily select which component ends up in which package. So I would delay this until an actual user community starts to develop.

@weidel-p https://github.com/weidel-p @mdjurfeldt https://github.com/mdjurfeldt Did you already define some kind of protocol for ZMQ + SNN? I just began a few weeks ago to use ZMQ to keep data persistence and plotting out of the loop, so I would be interested in your insights. It does not really make sense for me to develop a completely custom protocol for transportation of spiking data when another group does the same.

This is right now entirely Philipp's work, but I've told Philipp that there is a plan to completely refactor MUSIC such that MUSIC can natively and simultaneously use different communication methods. Communication methodss, like MPI, UDP and ZMQ will be modules which can optionally be compiled into the MUSIC library and can be selected in the MUSIC configuration.

Once this happens, I will study what you have done and maybe we can together decide how it should look like in MUSIC. For now, I think I will be too much distracted if I think about this, so I think you two should collaborate on this independently of myself.

I understand. I agree that making MUSIC more flexible regarding the method of communications is the right way to go!

As it looks now, the refactorization might actually start to happen already late this spring.

Great to hear!

BTW, Michael, I think you are doing great work and I'm sorry that there was a long period of unresponsiveness on my part i the beginning of the year.

Thank you. I appreciate that. :)

@weidel-p
Copy link
Owner Author

@mhoff @mdjurfeldt sorry for this delayed response. I was horribly busy lately.

So, I thought about this discussion and actually it's true what @mhoff says. Large parts of the adapters are redundant. It would be easier to use and develop if we would do exactly what @mhoff suggests.

To summarize the idea:

  • We create an organization for this project.
  • In this organization we create several repositories:
    • core (only dependent on MUSIC, contains shared libraries and abstract classes for adapters)
    • basic_adapters (implements basic adapters like encoders and linear_decoder)
    • ros_adapter (dependent on ROS, implements adapters to ROS publisher and subscriber)
    • dvs_adapter (dependent on ROS and DVS, implements adapters to the DVS camera)
    • zmq_adapter (dependent on ZMQ, implements adapters for ZMQ publisher and subscriber)
    • meta (contains all other repositories as submodules)

Did I understand that correctly or did I miss something?

The 'core' as well as the 'basic_adapters' could actually live in the MUSIC repository. But it might be better to think about that as soon as we implemented that.

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Mar 30, 2017 via email

@mhoff
Copy link
Contributor

mhoff commented Mar 30, 2017

To summarize the idea:

  • We create an organization for this project.
  • In this organization we create several repositories:
    • core (only dependent on MUSIC, contains shared libraries and abstract classes for adapters)
    • basic_adapters (implements basic adapters like encoders and linear_decoder)
    • ros_adapter (dependent on ROS, implements adapters to ROS publisher and subscriber)
    • dvs_adapter (dependent on ROS and DVS, implements adapters to the DVS camera)
    • zmq_adapter (dependent on ZMQ, implements adapters for ZMQ publisher and subscriber)
    • meta (contains all other repositories as submodules)

Did I understand that correctly or did I miss something?

My understanding was to put basic_adapters in MUSIC, and core, ros_adapter, dvs_adapter in a separate project "ros-music" (could be just one repository or multiple repositories), which can be simply dropped into the catkin workspace.

Your approach goes on step further. You allow the basic_adapters to use core functionality, which does perfectly make sense. I did not think about eliminating this redundancy. But I see issues regarding the build system. As core, basic_adapters and zmq_adapters are completely ROS-free, how do you intend to build those projects? I assume you don't want to enforce a catkin workspace for non-ROS users, thus ROS users would have to clone the meta module, build the non-catkin modules by hand and leave the rest to catkin. I think we have to come up with a smart solution for this problem.

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Mar 30, 2017 via email

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Mar 30, 2017 via email

@mhoff
Copy link
Contributor

mhoff commented Mar 30, 2017

Let me create an example:
As a user who uses MUSIC, ROS, DVS and ZMQ, I would need to install the following things

  • MUSIC in .source/music
  • music_adapter (core + basic_adapters) in .source/music_adapter.
  • zmq_adapter in .source/music_zmq_adapter
  • ros_music (ros_adapter + dvs_adapter) in $CATKIN_WS/src/ros_music

I think we should include zmq_adapter in music_adapter maybe with a compilation flag (e.g. -Dwith-zmq=ON). catkin is limited in this, but cmake allows for full flexibility in this extent.

As a consequence, we would have three repositories:

  • MUSIC (autotools?)
  • music_adapter (cmake)
    • core
    • basic_adapter
    • zmq_adapter (-Dwith-zmq=ON)
  • ros_music (meta)
    • ros_adapter (catkin)
    • dvs_adapter (catkin) (submodule/spare/CATKIN_IGNORE/...)

I would avoid creating too much repositories and working with git-submodules too much (people hate submodules).

@weidel-p
Copy link
Owner Author

@mhoff your example seems to be quite right. As mentioned already, we could also think about merging all ROS independent things in the MUSIC repository eventually. Then we would end with only two repositories: MUSIC and ros_music.

@mdjurfeldt Does the organization 'MUSIC' already exist? Otherwise I'll create it. Or do you mean INCF?

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Mar 30, 2017 via email

@mhoff
Copy link
Contributor

mhoff commented Mar 30, 2017

@weidel-p I think it's better to keep the different extensions in different repositories, possibly with the exception of basic/core. That makes the repositories leaner and can compartmentalize issues and PRs.

True. Does it follow that zmq_adapter should be a submodule to music_adapter? It certainly makes sense thinking about issues and PRs. The overhead introduced would come down to having to commit & push twice to make a change in zmq_adapter present in music_adapter. However, pulling changes in music_adapter becomes harder, as you also have to update the submodules every time.

The main MUSIC repo is INCF/MUSIC. Possibly, it could move to MUSIC/MUSIC in the future. (Maybe MUSIC is a too generic name for an org, btw? :)

Way too generic if you ask me. But do you have a viable alternative as organization name?
But I do strongly agree that we definitely need to group the repositories which will emerge in a dedicated organization or team.

@weidel-p
Copy link
Owner Author

weidel-p commented Apr 4, 2017

Another possibility would be to use the "official" INM-6/IAS-6 organization (https://github.com/INM-6). I think this would make sense. What do you think?

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Apr 4, 2017 via email

@weidel-p
Copy link
Owner Author

weidel-p commented Apr 4, 2017

I actually don't mind the name of the organization. INCF-MUSIC is fine for me. @mhoff do you have a name in mind?

@mhoff
Copy link
Contributor

mhoff commented Apr 4, 2017 •
edited
Loading

Before deciding on the concrete name I would like to discuss the letter case.

How could names for the repositories look like?

  • music
  • MUSIC
  • music-adapter
  • music_adapter
  • MUSIC_ADAPTER
  • MUSIC-adapter

In my opinion, mixed variants simply look bad and -- generally -- lower case variants look better than their upper case alternatives. As a popular example, NEST also chose the lower case variant for both, organization and repository name: https://github.com/nest/nest-simulator. The alternative would have been a worse choice if you ask me: https://github.com/NEST/NEST-simulator.

Assuming you agree with lower case in the repository name, the organization name should reflect this design choice. Consequently, we would speak of music/music, INCF-music/music, incf-music/music etc., whereas I don't know if it would be wise to use INCF in the lower case variant if that is unusual. Personally, looking at these choices, I'd actually prefer the general music/music (if music is still free as an origanization!).

Did you ever think about putting a suffix to music like nest did with nest-simulator? What about music-coordinator?

@mhoff
Copy link
Contributor

mhoff commented Apr 5, 2017

"music" is already taken as organization name..

@weidel-p
Copy link
Owner Author

Usually I would prefer lower letters, but assuming that we take INCF-MUSIC as organization name and as MUSIC and INCF in already written in capital letters, I would stick to it.

@mdjurfeldt regarding sub-organizations: we could just create a team in the INCF organization?

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented Apr 11, 2017 via email

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented May 18, 2017 via email

@weidel-p
Copy link
Owner Author

I'm fine with the names of the repositories and agree on the '-' convention.
I think we cover everything we need for now with these repos. Please proceed. :)

@mhoff
Copy link
Contributor

mhoff commented May 19, 2017 •
edited
Loading

I agree, we can certainly proceed with creating the repos :)

@mdjurfeldt
Copy link
Collaborator

mdjurfeldt commented May 19, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mdjurfeldt @mhoff @weidel-p