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

Release vp1 #255

Merged
merged 9 commits into from
Mar 4, 2021
Merged

Release vp1 #255

merged 9 commits into from
Mar 4, 2021

Conversation

jasmainak
Copy link
Collaborator

@jasmainak jasmainak commented Jan 22, 2021

closes #256

@codecov-io
Copy link

codecov-io commented Jan 22, 2021

Codecov Report

Merging #255 (637c0e7) into master (6a75dc7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #255   +/-   ##
=======================================
  Coverage   47.66%   47.66%           
=======================================
  Files          25       25           
  Lines        3151     3151           
=======================================
  Hits         1502     1502           
  Misses       1649     1649           
Impacted Files Coverage Δ
hnn_core/viz.py 4.68% <ø> (ø)
hnn_core/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a75dc7...7a189ee. Read the comment docs.

setup.py Show resolved Hide resolved
Copy link
Collaborator

@cjayb cjayb left a comment

Choose a reason for hiding this comment

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

This is looking good, made some minor suggestions.

pip installer:

$ pip install NEURON
and it will install ``hnn-core`` along with the dependencies which are not already installed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
and it will install ``hnn-core`` along with the dependencies which are not already installed.
which will install ``hnn-core`` along with the dependencies that are not already installed. The most notable of these is NEURON `https://neuron.yale.edu/neuron/ <https://neuron.yale.edu/neuron/>`_.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thought we should mention NEURON on the start page (or is it just "Neuron"?). EDIT: I see we have the link below in a different context... Might be nice to acknowledge here too, though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm hesitant to provide the link because users may go for the traditional install if they open the website and that will cause us unnecessary headaches

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought we should mention NEURON on the start page (or is it just "Neuron"?).

I'm a fan of NEURON to avoid confusion with the journal.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
doc/conf.py Outdated Show resolved Hide resolved
jasmainak and others added 3 commits March 1, 2021 09:11
Co-authored-by: Christopher J. Bailey <bailey.cj@gmail.com>
than one CPU core, refer to `parallel_backends`_

**Note for Windows users**

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We recommend using windows subsystem for linux (WSL). For use with pure Windows,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you have a Windows machine to give this a shot?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! Stuck in meetings today but I can give it a go later tonight for windows and linux.

Copy link
Contributor

@ntolley ntolley Mar 1, 2021

Choose a reason for hiding this comment

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

image

This showed up on the Windows install. You have to navigate to /Lib/site-packages/hnn_core/mod/ in your conda environment and run nrnivmodl manually. Are they supposed to compile automatically for all systems?

Also I'll check out mpi4py for windows, but I suspect it's going to be a power user feature...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nopes, it should work out of the box. That's the whole point of all this pain we're taking :) For linux/mac, it works out of the box for you, right?

so here is where nrnivmodl is called during the setup. Can you verify if this works? I remember having some trouble printing or adding breakpoints in the setup file, so you might need to do some googling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry I just pushed a commit to the branch, should I revert it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh indeed, sorry I had forgotten you could push. It's all good. Uploaded to test.pypi again. Give it a shot

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm that didn't seem to cut it. Perhaps there's a lag for test.pypi to update? Not sure how to check that since setup.py is not included in the pip install.

I'll try again in the morning, also I need to read through this but this may explain the issue:
https://docs.python.org/3/install/#how-installation-works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just download the tar from pypi directly and you should have the setup file there: https://test.pypi.org/project/hnn-core/#files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also if you tell me your test.pypi username I can add you there so you can try uploading yourself


**Note for Windows users**

The pip installer for Neuron does not yet work for Windows. In this case, it is better to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The pip installer for Neuron does not yet work for Windows. In this case, it is better to
the pip installer for Neuron does not yet work for Windows. In this case, it is better to

@jasmainak
Copy link
Collaborator Author

jasmainak commented Mar 1, 2021

okay I just uploaded on test.pypi to see if the sdist approach works. Can someone (other than me) give it a shot on:

  • Linux (tested by Mainak on CentOS 7.9)
  • Mac (tested by Mainak on Mac BigSur)
  • Windows
$ pip install --index-url https://test.pypi.org/simple/ hnn-core

@cjayb
Copy link
Collaborator

cjayb commented Mar 2, 2021

Tried following on macOS Catalina (edit: and on Ubuntu 16.04, identical results)

conda create -n hnn_pip python=3.7
conda activate hnn_pip
pip install --index-url https://test.pypi.org/simple/ hnn_core

Resulting in

Looking in indexes: https://test.pypi.org/simple/
Collecting hnn_core
  Downloading https://test-files.pythonhosted.org/packages/88/e1/818763673c61c0b22823ca3d9e36ed03ef84ae53c56bad50b63521fb8389/hnn-core-0.1.3.tar.gz (100 kB)
     |████████████████████████████████| 100 kB 2.9 MB/s
  Downloading https://test-files.pythonhosted.org/packages/c4/eb/c4266a4e53828cde9bcd33074177e0d7f3951265f15fd84507d4402eac14/hnn-core-0.1.2.tar.gz (100 kB)
     |████████████████████████████████| 100 kB 3.8 MB/s
  Downloading https://test-files.pythonhosted.org/packages/7d/18/40e2db1956d14cae0d990bd7631325690777100aed1ea23a11a27145c413/hnn-core-0.1.1.tar.gz (224 kB)
     |████████████████████████████████| 224 kB 4.0 MB/s

So no dependencies were downloaded, and it didn't even attempt to compile the mechanisms :( My username on test.pypi is cjayb...

@ntolley
Copy link
Contributor

ntolley commented Mar 2, 2021

Looking at setup.py, is it just because build_mod is only defined but never called? I'm not totally sure is the setup() function is supposed to do that automatically or not.

Doesn't explain why @jasmainak had successful installs so it may be throwing an error silently like on Windows 10.

@jasmainak
Copy link
Collaborator Author

So no dependencies were downloaded, and it didn't even attempt to compile the mechanisms

why do I see only downloading and no installing message logs? Are you sure there wasn't an existing version of hnn_core available in your environment that needed to be removed first

@jasmainak
Copy link
Collaborator Author

Looking at setup.py, is it just because build_mod is only defined but never called?

It's used here: https://github.com/jonescompneurolab/hnn-core/blob/master/setup.py#L107

@jasmainak
Copy link
Collaborator Author

@cjayb can you verify that after this step

conda activate hnn_pip

you can't import hnn_core in ipython

@ntolley
Copy link
Contributor

ntolley commented Mar 2, 2021

Looking at setup.py, is it just because build_mod is only defined but never called?

It's used here: https://github.com/jonescompneurolab/hnn-core/blob/master/setup.py#L107

That's what I mean, I was under the impression that pip internally runs setup.py install. Does it automatically use the commands defined in cmdclass as well? This comes to mind because the way I tested my most recent push is by running setup.py build_mod

@jasmainak
Copy link
Collaborator Author

jasmainak commented Mar 2, 2021

I was under the impression that pip internally runs setup.py install

right this is what I think too. And the "install" command in the setup file calls the "build" command which in turn calls the "build_py" and "build_mod" commands I believe. And "build_py" and "build_mod" can be overridden through the cmdclass. I guess if bdist is already provided, the "build" command might be skipped. All this is not very well documented ... I had to learn by reading the source code and stackoverflow

Do also read this and the rest of the comments on this thread. That's when I was learning how all this works.

@cjayb
Copy link
Collaborator

cjayb commented Mar 2, 2021

Some progress here. Part of our problem is that we're testing a non-standard installation (via test.pypi). Setting the --index-uri means that pip can't find the dependencies! A simple fix was to use --extra-index-url instead:

pip install --force-reinstall --extra-index-url https://test.pypi.org/simple/ hnn_core

This pulls in all the dependencies (yay!) and triggers a build (yay^2!), but alas:

  => Building mod files ...
  error: [Errno 2] No such file or directory: 'nrnivmodl': 'nrnivmodl'

which is because hnn_core is running its build before NEURON (and nrnivmodl) are installed. Simply re-running the above pip-line then completes and the installation is done.

Haven't been able to figure out how to force the dependencies to be installed before build, any thoughts? We're so close... :)

@cjayb
Copy link
Collaborator

cjayb commented Mar 2, 2021

Based on some digging (here, here, here and of course here ), 7a189ee seems to work on my machine locally! @jasmainak try uploading to test.pypi?

@jasmainak
Copy link
Collaborator Author

Just uploaded again to test.pypi! This is turning out to be quite the teamwork :)

@cjayb
Copy link
Collaborator

cjayb commented Mar 2, 2021

Yeah!

  • macOS Catalina, including build
  • Ubuntu 16.04

Edit: that is, the following now builds and examples run:

conda create -n hnn_pip python=3.7
conda activate hnn_pip
pip install  --extra-index-url https://test.pypi.org/simple/ hnn_core

@cjayb
Copy link
Collaborator

cjayb commented Mar 2, 2021

also maybe need to update MANIFEST.in to include it in the dist/?

I don't think this is needed: the file is only relevant for installation using setuptools, then safe to ignore. Unless of course it's somehow relevant when upgrading.

@jasmainak
Copy link
Collaborator Author

okay so here is the game plan I propose:

  • @ntolley tests this one Windows and tells us if it works/ if it needs further changes. But I guess we don't want to spend too much time on it. If we can get it to work in a day or two that's good. Otherwise, I suggest we go for a release with only Linux and Mac support for now. It would be great to have it working though.
  • @rythorpe makes the wordsmithing PR on the somatosensory example with links to HNN tutorials. This is not urgent though. The website can always be updated later ... we will however freeze the website at the 0.1 release or after Ryan makes the PR. So that users don't get confused by mismatch between examples and hnn-core version.
  • you guys approve this PR when it's good by you
  • I'll self-merge (because I want to be 100% sure myself that everything works) and make the release on pypi.

Comment on lines +2 to +3
requires = ["setuptools>=40.8.0", 'NEURON >=7.7']
build-backend = "setuptools.build_meta:__legacy__"
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, I don't understand the implications of the version requirement (Feb 2019) nor the __legacy__ flag. I guess it's a safe bet that our pip-based users will be fine with both.

@ntolley
Copy link
Contributor

ntolley commented Mar 3, 2021

Windows install is proving finnicky. Here is the output from pip install --no-deps https://test.pypi.org/simple/ hnn_core -vvv
image

It's not very obvious to me where this is failing as I thought the directory error was resolved with my more recent commit. Not sure if I'll be able to troubleshoot too intensely until next week so it may be worth holding off on the windows release? I'm hoping it's a fairly straightforward discrepancy in the windows directory but hard to say at the moment.

@jasmainak
Copy link
Collaborator Author

okay let's move forward without Windows. If we do find a fix later, we can backport it. I'll let some Windows user complain before fixing it :) FWIW, we did make it work before with @kohl-carmen in the development environment. But unfortunately my antiquated Windows machine is complaining too much now ... so I'll skip this step for now.

@jasmainak jasmainak merged commit 99789af into jonescompneurolab:master Mar 4, 2021
@jasmainak
Copy link
Collaborator Author

alright folks, single line install of hnn-core is here.

$ pip install hnn-core

Enjoy Mac and Linux users!

@jasmainak jasmainak mentioned this pull request Mar 4, 2021
@rythorpe
Copy link
Contributor

rythorpe commented Mar 5, 2021

@rythorpe makes the wordsmithing PR on the somatosensory example with links to HNN tutorials. This is not urgent though. The website can always be updated later ... we will however freeze the website at the 0.1 release or after Ryan makes the PR. So that users don't get confused by mismatch between examples and hnn-core version.

I've been in manuscript-writing land and am just getting caught up now - sorry for the very late reply. I'll try to make a PR by the end of this weekend. I'm super excited about this release, it's been a long time coming @jasmainak!

@jasmainak
Copy link
Collaborator Author

sounds good! You might have to do something like 1a1072d if you want to push the changes to 0.1 documentation.

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.

error when installing hnn-core
5 participants