Bug 1273579 - Review Request: nest - The neural simulation tool
Review Request: nest - The neural simulation tool
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
NOTREADY
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-20 14:00 EDT by Ankur Sinha (FranciscoD)
Modified: 2016-04-26 14:20 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ankur Sinha (FranciscoD) 2015-10-20 14:00:26 EDT
Spec URL: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec
SRPM URL: https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-1.gitdc21fdc.fc23.src.rpm

Description: 
NEST is a simulator for spiking neural network models that focuses on the
dynamics, size and structure of neural systems rather than on the exact
morphology of individual neurons. The development of NEST is coordinated by the
NEST Initiative.

NEST is ideal for networks of spiking neurons of any size, for example:

    - Models of information processing e.g. in the visual or auditory cortex of
mammals,
    - Models of network activity dynamics, e.g. laminar cortical networks or
balanced random networks,
    - Models of learning and plasticity.


Fedora Account System Username: ankursinha
Comment 1 Ankur Sinha (FranciscoD) 2015-10-20 14:07:03 EDT
Copr builds here:

https://copr.fedoraproject.org/coprs/ankursinha/neuroscience-research/package/nest/
Comment 5 Raphael Groner 2015-11-13 12:49:08 EST
Are you interested in a review swap? Maybe mdp (rhbz#1246790) or polyglot (rhbz#1197333) are something for you to look at.
Comment 6 Raphael Groner 2015-11-13 13:00:18 EST
Review swap maybe with mdp (bug #1246790) or polyglot (bug #1197333)?
Comment 7 Ankur Sinha (FranciscoD) 2015-11-16 08:32:41 EST
Taken them up. These are the latest spec/srp links:

https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest.spec

https://ankursinha.fedorapeople.org/neuroscience-research-copr/nest-2.8.0-5.gitdc21fdc.fc23.src.rpm

I'm using the packages with openmpi now, and they seem to work fine. Some work may be needed to make the package better, though :)

Thanks!
Ankur
Comment 8 Raphael Groner 2015-11-16 13:49:30 EST
For a first step, I found some general advices to your spec file.

MUST:
- Add full URL to Source0, it's not sufficient to mention in comment only.
  I guess you use the snapshot tarball of GitHub, then please follow the 
  official recommendation to do so. Alternatively, you have to specify in a
  comment how to create the tarball (with an included script in SRPM?).
  https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services
- Did you send the patches to upstream? Please make notes in comments or
  use the direct URLs.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Patch_Guidelines
  https://fedoraproject.org/wiki/Staying_close_to_upstream_projects#Why_push_changes_upstream.3F
- Some lines in %description seem to be too long. Please restructure this block.
  The same counts also for the subpackages analogously.
- Usage of %global is preferred over %define, please explain why this does not
  apply here.
  https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
- The common subpackage does not contain the license file, please add it to
  satisfy all other subpackages with that license too through the specified 
  dependencies. The doc subpackage needs also the license file.
- Use the new %python_provide placeholder, the Provides are somehow wrong when
  we follow the new guidelines.
  https://fedoraproject.org/wiki/Packaging:Python#Provides
- The subpackages have to own also the individually created sub folders, 
  besides the files itself.
  https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_wholly_contained_in_your_package.2C_or_involves_core_functionality_of_your_package
- Specification of doc subpackage is not allowed as such.
  It's needed to list all folders and files individually, of course wildcards
  are possible. Remove %doc when you directly use installed pathes.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

SHOULD:
- You can use pushd/popd instead of cd commands to get the path changes logged.
  http://www.eriwen.com/bash/pushd-and-popd/
- Use "cp -p" to preserve timestamps.
- Please try harder to disable the check of automake, it's better with a 
  special patch (maybe applied in epel7 only but you do not need this build 
  conditional here at all), instead of this ugly sed command that is going to 
  break in future when the line numbers change.
  https://fedoraproject.org/wiki/Packaging:Guidelines#Applying_patches
- You can use the new macros %make_build and %make_install that set default 
  parameters.
- Please rename the subpackages as suggested in the example template:
  python2-nest, python2-nest-openmpi and python2-nest-mpich
  https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file
- Please ask upstream about future plans to support Python3.
  https://fedoraproject.org/wiki/Packaging:Python#Python_Version_Support

That's it for now. When the above hints are fixed, I can take a deeper look.
Comment 9 Ankur Sinha (FranciscoD) 2016-01-05 06:24:39 EST
Heya,

So, upstream confirmed that nest supports py3, but they've also said that they intend to redo the build system in the future. If the build system is redone, I'll have to pretty much redo the package - do you think it makes sense to hold off the review until they've completed the change? (I'll keep up with upstream and maintain the COPR until then)

Cheers,
Ankur
Comment 10 Zbigniew Jędrzejewski-Szmek 2016-01-05 19:04:53 EST
> do you think it makes sense to hold off the review until they've completed the change?

I don't think that waiting for some hypothetical future upstream changes makes much sense. At most one of the issues in comment #8 touches the build system (the configure sed, and I don't think Raphael is right here, I think sed is as good as a patch, and less work), they are all about package naming and descriptions and other stuff which is pretty much orthogonal to the build system. F24 will be a very attractive release for neuroscience with various neew packages added, and I'd love to see nest on that list.
Comment 11 Ankur Sinha (FranciscoD) 2016-01-08 10:46:50 EST
It isn't actually a hypothetical upstream change. The nest user mailing list has received clear emails that detail the move to NEST3, which they say is a complete overhaul of the code base - while maintaining compatibility, of course - but it is quite possible that the build system changes too. 

I'm happy to continue the review if another reviewer comes along, but I do think a re-review will be in order (even if unofficially) if the build system and change. 

Is the plan to get all the neuroscience packages into Fedora by F24, though - or can we have a "group copr" thing running and transition packages over as they pass review?
Comment 12 Zbigniew Jędrzejewski-Szmek 2016-01-08 12:50:58 EST
(In reply to Ankur Sinha (FranciscoD) from comment #11)
> I'm happy to continue the review if another reviewer comes along,
I don't understand this sentence. Raphael said that he can review the package in comment #8. If he cannot do it, I'm sure that somebody else (e.g. me) would step up.

> but I do
> think a re-review will be in order (even if unofficially) if the build
> system and change. 
Nah, the build system is not visible in the binary package. As long as it builds nobody cares (except the maintainer of course).

> Is the plan to get all the neuroscience packages into Fedora by F24, though
> - or can we have a "group copr" thing running and transition packages over
> as they pass review?

I don't think there's a plan to get "all" neuroscience packages into Fedora, there's probably too many to even consider that. https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific list. New packages can be added at any time, and there's quite a bit of time before Fedora 24 release (2016-05-17). I don't know about any plans to have a group copr. I see coprs as a good mechanism to provide alternative versions of already packaged software, or often updated software, or things which are inappropriate for main Fedora for other reasons. I don't think nest or other science related software falls into any of those categories, and making a "detour" through copr would be mostly a waste of time.
Comment 13 Ankur Sinha (FranciscoD) 2016-01-08 14:13:18 EST
(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> (In reply to Ankur Sinha (FranciscoD) from comment #11)
> > I'm happy to continue the review if another reviewer comes along,
> I don't understand this sentence. Raphael said that he can review the
> package in comment #8. If he cannot do it, I'm sure that somebody else (e.g.
> me) would step up.

Oh, he dropped the review after I asked his opinion about the build system change in comment 9 which I took as an indication that he agreed with me putting off the package (He isn't the assignee any more, and he isn't in the CC list either) :)

> 
> > but I do
> > think a re-review will be in order (even if unofficially) if the build
> > system and change. 
> Nah, the build system is not visible in the binary package. As long as it
> builds nobody cares (except the maintainer of course).

That is what I meant - when a new build system comes, the entire package has to be effectively redone, and nest, as you'll see from the spec, isn't the simplest of packages. Totally a maintainers burden, yes, but I'm going to be the maintainer ;)

> 
> > Is the plan to get all the neuroscience packages into Fedora by F24, though
> > - or can we have a "group copr" thing running and transition packages over
> > as they pass review?
> 
> I don't think there's a plan to get "all" neuroscience packages into Fedora,
> there's probably too many to even consider that.
> https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific
> list. New packages can be added at any time, and there's quite a bit of time
> before Fedora 24 release (2016-05-17). I don't know about any plans to have
> a group copr. I see coprs as a good mechanism to provide alternative
> versions of already packaged software, or often updated software, or things
> which are inappropriate for main Fedora for other reasons. I don't think
> nest or other science related software falls into any of those categories,
> and making a "detour" through copr would be mostly a waste of time.

I think the list is somewhere on a google spreadsheet. I don't have the link handy at the moment. 

Coprs are also a good mechanism for packages that build and are functional but are not packaged well enough to pass a formal review yet (outer ring in fedora.next and all that), which is exactly what the nest package is at the moment.

Anyway, I'm happy to continue the review if someone takes it up. I'll update the spec with Raphael's comments later this week. I already updated the copr package to 2.10 which came out recently. I need to work on the python3 bit etc.

Cheers!
Ankur
Comment 14 Zbigniew Jędrzejewski-Szmek 2016-01-08 14:40:29 EST
(In reply to Ankur Sinha (FranciscoD) from comment #13)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> > (In reply to Ankur Sinha (FranciscoD) from comment #11)
> > > I'm happy to continue the review if another reviewer comes along,
> > I don't understand this sentence. Raphael said that he can review the
> > package in comment #8. If he cannot do it, I'm sure that somebody else (e.g.
> > me) would step up.
> 
> Oh, he dropped the review after I asked his opinion about the build system
> change in comment 9 which I took as an indication that he agreed with me
> putting off the package (He isn't the assignee any more, and he isn't in the
> CC list either) :)
Was he ever? I only see comment #8 from him.
Anyway, like I said, I can review the package.

> > > but I do
> > > think a re-review will be in order (even if unofficially) if the build
> > > system and change. 
> > Nah, the build system is not visible in the binary package. As long as it
> > builds nobody cares (except the maintainer of course).
> 
> That is what I meant - when a new build system comes, the entire package has
> to be effectively redone, and nest, as you'll see from the spec, isn't the
> simplest of packages. Totally a maintainers burden, yes, but I'm going to be
> the maintainer ;)
True. I doesn't seem so bad to me (about normal for an mpi package in fact :)),
but of course I can't and don't want to push you.

> > > Is the plan to get all the neuroscience packages into Fedora by F24, though
> > > - or can we have a "group copr" thing running and transition packages over
> > > as they pass review?
> > 
> > I don't think there's a plan to get "all" neuroscience packages into Fedora,
> > there's probably too many to even consider that.
> > https://fedoraproject.org/wiki/Changes/NeuroFedora doesn't have a specific
> > list. New packages can be added at any time, and there's quite a bit of time
> > before Fedora 24 release (2016-05-17). I don't know about any plans to have
> > a group copr. I see coprs as a good mechanism to provide alternative
> > versions of already packaged software, or often updated software, or things
> > which are inappropriate for main Fedora for other reasons. I don't think
> > nest or other science related software falls into any of those categories,
> > and making a "detour" through copr would be mostly a waste of time.
> 
> I think the list is somewhere on a google spreadsheet. I don't have the link
> handy at the moment.
I wasn't aware of that. I know there's
http://taiga.fedorainfracloud.org/project/ignatenkobrain-neurofedora/kanban
but probably not everything is on there.

> Coprs are also a good mechanism for packages that build and are functional
> but are not packaged well enough to pass a formal review yet (outer ring in
> fedora.next and all that), which is exactly what the nest package is at the
> moment.
In my experience, it's quite easy for coprs to get obsolete. If the package
is in Fedora proper, than at least it will be regularly rebuilt, sometimes
a provenpackager will do a drive-by fix of some small issue, and users can
report issues in bugzilla, crashes are colleted. I think coprs are great
for development, or for a small focuses group of users, but after using them
for a while, I'm starting to see downsides of using them as a mechanism for
distributing software to a diverse group of users, over long time.

> Anyway, I'm happy to continue the review if someone takes it up. I'll update
> the spec with Raphael's comments later this week. I already updated the copr
> package to 2.10 which came out recently. I need to work on the python3 bit
> etc.

Great! Let's see if Raphael wants to pick up the review. If not, I'll take it.
Comment 15 Ankur Sinha (FranciscoD) 2016-02-02 11:50:02 EST
Pull request with cmake build system: https://github.com/nest/nest-simulator/pull/213

I suggest we hold off until this is merged?
Comment 16 Ankur Sinha (FranciscoD) 2016-04-25 16:03:45 EDT
So, I've started working on the cmake spec. I have a question though - I need to build it without mpi, with mpich, with openmpi, and for each of these 3, with python2 and python3 - so, can I just build the thing SIX times in the spec?? I've been trying to hack around and try to limit it to the minimum three times - without mpi, with mpich and with openmpi, but it's extremely hacky and quite frankly ugly. Thoughts?
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-04-26 09:37:56 EDT
I'm don't think you need to build 6 times. The python wrapper might be independent of mpi. Can you post your latest version of the spec file, I'll try to have a look. If the python module (.so files in %{python2_sitearch}/%{name}) aren't linked to libmpi, even if the package is built with mpi, then there's no need to provide separate python2-openmpi, python2-mpich, etc, subpackages.
Comment 18 Ankur Sinha (FranciscoD) 2016-04-26 11:46:17 EDT
This is the current version, which doesn't quite work:

https://ankursinha.fedorapeople.org/misc/nest.spec

They use cmake, which builds the c++ bits to build the SLI version. The cmake then calls python setup.py to build pynest and the other python requirements - connplotter, topology. For the python bit, they haven't separated building and installing, so I patched that part out and was doing it myself. The cython part works OK using the cmake build system itself. I don't think thee py parts link to MPI, but I'll have to check. If they don't, we can probably do:

- build serial with python2
- build serial with python3
- build mpich without python
- build openmpi without python

That'll simplify the build quite a bit.
Comment 19 Ankur Sinha (FranciscoD) 2016-04-26 11:47:52 EDT
Oh, by the way, this version gets the latest cmake version of the code from this pull request: https://github.com/nest/nest-simulator/pull/318

I've put up the srpm too: https://ankursinha.fedorapeople.org/misc/nest-2.10.0-22.git79b2f01.fc23.src.rpm
Comment 20 Zbigniew Jędrzejewski-Szmek 2016-04-26 14:20:02 EDT
Python .so files have rpath set, you'll need to get rid of it [https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath]. I haven't look here, but sometimes -DCMAKE_SKIP_RPATH:BOOL=ON works with cmake.

I had to modify the spec file to triple the backslashes in dobuild macro for all the cmake args (cmake \\\
                    -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \\\
                    ...)
With that change it builds and installs and crashes in debuginfo generation.

I think you should change all ";" to "&&" in %dobuild, otherwise errors might go unnoticed.

(In reply to Ankur Sinha (FranciscoD) from comment #18)
> - build serial with python2
> - build serial with python3
> - build mpich without python
> - build openmpi without python

Yeah, that would be the best option. I'm still not sure if that's possible.
./usr/lib64/python{2.7,3.4}/site-packages/nest/pynestkernel.so
links to ./usr/lib64/libnestkernel.so. If the MPI version is built,
it might be possible to link the same pynestkernel.so to
/usr/lib64/openmpi/lib/libnestkernel.so if it has the same symbols.

But I think the spec file looks 80-90% done...

The python subpackages should be called python2-nest and python3-nest
(they can have Provides: pynest). This seems more in line with modern
practice, and the guidelines [https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Addon_Packages_.28python_modules.29].

%python_provide should be used [https://fedoraproject.org/wiki/Packaging:Python#The_.25python_provide_macro, https://fedoraproject.org/wiki/Packaging:Python#Example_common_spec_file has an actual example].

Some more recommendations:
- I wouldn't bother with a seperate -connplotter subpackage. People
  are more likely to install and use it with the pynest anyway, and it
  would simplify packaging a bit.

- Do you really need openmpi-common and openmpi-doc? If they contain
  the same stuff as common and doc, you should use them instead.
  A symlink in the right place might be necessary.

- define a %global _description and reuse it in all %descriptions.
  Though -common and -doc probably don't need a full description.

- python2 and python3 subpackages must Require an exact version of the
  main package, because you don't want the user to mix pynestkernel.so
  with some different version of libnestkernel.so.

- openmpi or mpich python subpackages should not have Provides: python2-nest,
  because they are not interchangeable. (If some other package has a dependency
  on python2-nest, it'll break if python2-nest-openmpi is installed instead.)

Note You need to log in before you can comment on or make changes to this bug.