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

Update README.rst build instructions #101

Merged
merged 11 commits into from
May 10, 2020

Conversation

roryyorke
Copy link
Collaborator

  • note tests depend on scipy
  • remove reference to deleted conda-build
  • added conda instructions per platform (Linux, macOS, Windows)
  • remove instruction installing "plain" LAPACK from conda
  • minor rewording and fixes

@roryyorke
Copy link
Collaborator Author

@bnavigator please review; this should be as discussed in #98

Copy link
Collaborator

@bnavigator bnavigator left a comment

Choose a reason for hiding this comment

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

On line 80 it should read "Unpack the source code ..."

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@bnavigator
Copy link
Collaborator

The coveralls badge also needs updating (fixes #44):

.. image:: https://coveralls.io/repos/github/python-control/Slycot/badge.svg
   :target: https://coveralls.io/github/python-control/Slycot

@roryyorke
Copy link
Collaborator Author

@bnavigator thank you.

@bnavigator
Copy link
Collaborator

Quoting my own comments from the review above, so they don't get buried.

On line 80 it should read "Unpack the source code ..."

Actually, I have no idea, whether the windows builds work at all. The bld.bat in conda-recipe-openblas is unchanged from when it was still conda-recipe and uses environment variables for Generic BLAS/LAPACK. conda-forge/slycot-feedstock also has the same setup.

@moorepants, @murrayrm, @repagh you are maintaining the feedstock. Care to comment?

Should we move the Windows Script to conda-recipe-win/? Anybody got Slycot with MKL (with conda provided libs?) set up in Windows?

@bnavigator
Copy link
Collaborator

Discussion in #98 going on: We need to clarify the description how to build on MacOS X

@roryyorke
Copy link
Collaborator Author

conda-build-openblas doesn't work on my Windows system. First I hit this [1] (which I think we've discussed before); I added build-backend="setuptools.build_meta" to pyproject.toml, but then CMake couldn't find Visual Studio 2015, though it could find it when I built in a conda environment with python setup.py install (see #102).

I'm inclined to explicitly state we don't have a Windows conda build recipe; we can point to the conda-forge Slycot feedstock for anyone who wants to try to build their own conda package.

I also don't want to give detailed Windows build instructions; the script I have is hacky, and I suspect fragile.

I won't be able to give much time to this until Saturday. Meanwhile anyone wanting to wrap this PR up and make the 0.4.0 release is welcome to.

[1] pypa/setuptools#1694

@bnavigator
Copy link
Collaborator

I'm inclined to explicitly state we don't have a Windows conda build recipe; we can point to the conda-forge Slycot feedstock for anyone who wants to try to build their own conda package.

+1 for that. That means we should also remove the bld.bat in both conda recipe directories.

@roryyorke
Copy link
Collaborator Author

@bnavigator Is pytest (now?) a pre-requisite for running the unit tests? I saw you used a @pytest decorator in #104.

@bnavigator
Copy link
Collaborator

Yes, should probably add it to the various dependency declarations.

@bnavigator
Copy link
Collaborator

string: py{{ environ.get('PY_VER').replace('.', '') }}{{ environ.get('GIT_DESCRIBE_HASH', '') }}_mkl_{{ environ.get('GIT_DESCRIBE_NUMBER', 0) }}

Since you're already cleaning up the conda recipes in this PR, the _mkl_ infix needs to be changed to _apple_

@roryyorke
Copy link
Collaborator Author

I haven't done anything about #98 (comment):

#91, #92, #93 also have some back and forth discussion about getting Mac to build. Turns out it is crucial, that the correct MacOSX SDK is used. Maybe we should also add this to the README.rst in #101.

I'll skim the referenced issues / PRs, but otherwise suggestions for wording for this are weclome.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Show resolved Hide resolved
If you prefer to use the OpenBLAS library, a conda recipe is available in
``conda-recipe-openblas``.
On Linux you can choose between ``conda-recipe-openblas`` and
``conda-recipe-mkl``
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that there seems to be relatively new way for users to switch between blas versions: https://conda-forge.org/docs/maintainer/knowledge_base.html#blas

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's only for users getting their packages from conda-forge not for using conda-build locally, though? So rather something for the conda-feedstock package than for the instructions here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but it begs the question for me why Slycot wants to try to provide conda build instructions for non-defaults/conda-forge dependencies. Building a compatible set of conda binaries locally has to pull dependencies from somewhere or you have to build the whole stack locally. Mixing the underlying blas/lapack/etc libs with those in conda-forge and defaults is likely asking for trouble.

What is the reason for having these conda build scripts in slycot at this point in time? Doesn't the setup.py that's now powered by scikit build let users do custom installations sufficiently?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally agree, that the current mixing is asking for trouble.

The linked article implies that it should not matter against which library the build is linked during compile time, because the implementations should be ABI compatible. Maybe we are overcomplicating things here and if we still want to provide a recipe just give a single default one?

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 not sure why we maintain these conda-build recipes, especially given the existence of slycot-feedstock.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The recipes in the feedstock are normal conda recipes. I just tested on Ubuntu 20.04. I cloned the feedstock recipe, cd'd into the recipe directory, and did conda build -c conda-forge .. It built successfully and passed the tests. There are some parts of the recipe that are specific to conda forge, e.g. disabling the python 2.7 builds for windows, which a user would have to remove if they wanted to do that build.

OK, but at the top of the recipe is this:

{% set version = "0.3.5.0" %}
{% set sha256 = "cad98d5ea4f0a034cf398c39189f587620a0b03f1d4b71e77cd622a327f13adf" %}

package:
  name: slycot
  version: {{ version }}

source:
  url: https://pypi.io/packages/source/s/slycot/slycot-{{ version }}.tar.gz

which means 0."build 3.5 from PyPI", right? This is appropriate for the feedstock, but doesn't easily let one build other versions.

This is all OK, I just want to be sure I understand.

Copy link
Contributor

Choose a reason for hiding this comment

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

A user can change the version number and see if it builds, but if you want to make old builds you would likely need to checkout a prior commit in the feedstock recipe corresponding to a previous version. But, that said, keeping old builds working as time passes will get increasingly hard due to updates in conda, conda build, and any dependencies. You'd have to build with old versions of conda and put max versions on the dependencies, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the question is more about building a more recent version from the git repository instead of the released 0.3.5 (or hopefully soon 0.4.0). Even then it is just a matter of adjusting the source tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the general practice in conda forge is to point ot "official" releases on PyPi (so that it parallels what pip install does).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course, because that is what you want to have in the conda channel.

Now, if a user wants to compile a newer version of Slycot, say because there is a new feature in master or even in a PR that has not been released yet. Can he or she take the conda-forge recipe, use it for the checked out commit in question and install it in his or her environment? Sure. Question is, should we provide instructions for it or?

README.rst Show resolved Hide resolved
@repagh
Copy link
Member

repagh commented Apr 29, 2020

I think it would be best to refer to the windows build recipe on slycot-feedstock, and not have those in the slycot source code tree. It would only make sense if we have slycot developers who use windows for development. For me, it was and is a PITA to create the windows recipe, I will hack on the feedstock one for my students & the general public, but have no interest in maintaining a recipe in the slycot tree as well.

@murrayrm
Copy link
Member

I agree with the comment that we don't need to spend a lot of time on the Windows build if (a) we have condo-forge working and (b) nobody is developing on Windows. We can just refer interested parties to the slycot-feedstock if they want help.

I do think that we should keep the MacOS version working (see #98), but here also I would focus on what active developers are using (eg, we can delete conda-recipe-apple unless an active developer is using it). For me (not currently active, but sometimes a contributor), it is more useful to have the setup.py script working for MacOS versus a conda build script.

roryyorke and others added 7 commits May 2, 2020 13:35
- note tests depend on scipy
- remove reference to deleted conda-build
- added conda instructions per platform (Linux, macOS, Windows)
- remove instruction installing "plain" LAPACK from conda
- minor rewording and fixes
Authors found from `git shortlog -sn`.
For rename of CREDITS to AUTHORS, and previously removed runtests.py.
@roryyorke
Copy link
Collaborator Author

Is there consensus to remove all the conda recipes, and add a pointer in README.rst to slycot-feedstock for anyone needing such a recipe?

Along with this we would also change the Travis config to only use "setup.py" type builds.

Specify minimum scikit-build version.

Updated Windows build instructions for Python 3.7, 3.8, and to use
pytest.
README.rst Outdated Show resolved Hide resolved
@repagh
Copy link
Member

repagh commented May 4, 2020

I like to keep the conda recipes in. These can be a reference for the feedstock ones, and since conda-forge is an important distribution channel for slycot, I would prefer to have a conda build in the tests.

@roryyorke roryyorke merged commit 0cf9820 into python-control:master May 10, 2020
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.

5 participants