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

expose python strategy #109

Merged
merged 3 commits into from
May 3, 2018
Merged

expose python strategy #109

merged 3 commits into from
May 3, 2018

Conversation

casperdcl
Copy link
Member

@casperdcl casperdcl commented Apr 17, 2018

most of this (see original description below) is migrated to SyneRBI/SIRF#163

new description

original description

old

  • INSTALL/
    • python/
      • *.(py|so)

new

  • INSTALL/
    • python/
      • setup.py
      • sirf/
        • __init__.py
        • *.(py|so)
      • p*/__init__.py

future

@casperdcl casperdcl self-assigned this Apr 17, 2018
@KrisThielemans
Copy link
Member

hi. I didn't check this yet, but quite a few extra changes here related to Docker etc. intentional?

@casperdcl
Copy link
Member Author

casperdcl commented Apr 18, 2018

yes, it's some fairly no-brainer docker tidy up, would probably merge that in separately first (#110 )

@casperdcl casperdcl force-pushed the setup_py branch 2 times, most recently from 011a83a to f4ccbf6 Compare April 19, 2018 18:07
@casperdcl
Copy link
Member Author

casperdcl commented Apr 20, 2018

@paskino please note my comment in the description about using setup.py to call cmake - in which case we might be able to get away with

  1. install miniconda
  2. install requirements using apt-get (possibly almost nothing since most things might be on conda?)
  3. conda install -c conda-forge sirf (will auto-install dependencies, then run setup.py on sirf-superbuild which will run cmake and make)

@paskino
Copy link
Contributor

paskino commented Apr 20, 2018

Casper is setup.py already calling cmake to build sirf?

@casperdcl
Copy link
Member Author

No, but I could make it that way. Right now the procedure is:

  1. apt-get requirements (cmake, boost, etc)
  2. create python virtualenv and install python requirements
  3. cmake && make (will build everything as before, except for the following changes:)
    • PYTHONPATH is no longer altered anywhere
    • setup.py is generated and used in a pip install command to automatically pick up built python modules

@paskino
Copy link
Contributor

paskino commented Apr 20, 2018

Maybe that's not necessary then. conda recipe currently covers 1 and 2. If we split 3 in:

3.a cmake && make
3.b python setup.py install

I guess I could just merge this branch in the conda branch adding 3.b

@KrisThielemans
Copy link
Member

I don't see the benefit of splitting into 3a and 3b. "make" already calls setup.py. why split it?

I also don't think letting setup.py do cmake&&make is a good idea. It means you cannot build MATLAB and Python in one go.

(obviously, in the conda script you wouldn't want to call make but cmake --build -C Release or whatever the syntax is).

@paskino
Copy link
Contributor

paskino commented Apr 24, 2018

The conda built SIRF works, except #111

@casperdcl
Copy link
Member Author

btw, docker image sizes for different python sources:

  • virtualenv: 2.84GB
  • miniconda: 2.98GB
  • miniconda (update --all): 3GB

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

I feel that the SIRF specific stuff (i.e. 90%) should be in the SIRF repo itself, but not sure if that'd lead to a ton of duplication.
If we need to keep it here, it definitely needs to be in a separate file, presumably called from External_SIRF.cmake

@@ -0,0 +1,40 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

is this ok? shouldn't it use PYTHONINTERP?

Copy link
Member Author

Choose a reason for hiding this comment

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

should be fine, mainly just for correct syntax highlighting despite the .in extension - it's never executed directly (always ${PYTHON_EXECUTABLE} setup.py, not ./setup.py) so is ignored anyway

@paskino paskino mentioned this pull request Apr 26, 2018
This was referenced Apr 26, 2018
@casperdcl casperdcl changed the title sirf python packaging expose python strategy Apr 27, 2018
@casperdcl casperdcl merged commit 57c7dbf into master May 3, 2018
@casperdcl casperdcl deleted the setup_py branch May 3, 2018 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants