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

Add HDF5 #192

Merged
merged 7 commits into from
Mar 25, 2016
Merged

Add HDF5 #192

merged 7 commits into from
Mar 25, 2016

Conversation

groutr
Copy link
Contributor

@groutr groutr commented Mar 23, 2016

Split from #108

--with-pthread=yes --enable-cxx --with-default-plugindir=$PREFIX/lib/hdf5/plugin

make
make install
Copy link
Member

Choose a reason for hiding this comment

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

If there a make check?

@jakirkham
Copy link
Member

Thanks for putting this together @groutr.

features:
- vc9 # [win and py27]
- vc10 # [win and py34]
- vc14 # [win and py35]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need both this and python in build though.

Copy link
Member

Choose a reason for hiding this comment

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

You do. These are the actual features. Python in build only makes the selectors here work (I think).

Importantly, hdf5 should NOT be a python library - it is a lower-level library. This is why python is only a build-time dependency. If it were a runtime dependency, it would be tied to that particular version of Python. Here's a strange use case that would be possible with the former, but not the latter: build Python 2.7 with VS 2015. We did a very bad job coupling python with visual studio version too strongly. It is a strong convention, sure, but not a hard requirement. I hope I have time to correct the way that features are done soon - I will do something more explicit, like what @ukoethe has done in ilastik.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, thanks for setting me straight.

I had a nice chat with @ukoethe and @stuarteberg earlier. There are some proposals that @ukoethe has for how we may help address these sort of things in conda build and more generally ABI and API compatibility. Also, I think it would be great if we can we work with @ukoethe to incorporate his work on building Python with a looser VS constraint as a proposal at conda-forge.

Copy link
Member

Choose a reason for hiding this comment

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

I am having a deja-vu here. It is time to compile that info in a wiki! I will start something tomorrow.

Copy link
Member

Choose a reason for hiding this comment

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

@jakirkham I'm excited to have so many brilliant people thinking about these issues. I'll do what I can to help get @ukoethe's work with the looser constraints into conda-build.

Conda forge is a different story than conda-build, obviously. For conda-forge, the question I think is "do you want to leverage conda-forge to support non-standard Python distributions?" My sentiment is "probably," but at what cost in terms of taxing the build services? If that cost is acceptable, this would be really exciting. You could put together alternative non-standard distributions, like the one that @jjhelmus posted last week (ACPD?)

@jakirkham
Copy link
Member

Great stuff, @groutr. Added some minor comments. Thanks for helping looking at this @ocefpaf.

url: http://www.hdfgroup.org/ftp/HDF5/current/src/hdf5-{{version}}.tar.gz
md5: b8ed9a36ae142317f88b0c7ef4b9c618


Copy link
Member

Choose a reason for hiding this comment

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

Micro nit, only one blank line here.

@jakirkham
Copy link
Member

Restarting the Mac CI as it appears to have stalled out.

@jakirkham
Copy link
Member

One very minor nit. Otherwise I think I am 👍 on this with the nit fix and CI passing. Anyone else have any thoughts?

@jakirkham
Copy link
Member

After restarting the Mac build it died again, the error, at least IMHO, is a bit annoying. Travis CI doesn't like it if something doesn't produce output for 10 mins. While this can be the sign of a hang, there are perfectly reasonable scenarios where it isn't a hang, but a silent long running command. Unfortunately, there is no simple way to change the time limit (unless this has changed recently 🙏).

One way to workaround this is to use travis_wait, which is essentially a hack that writes to disk periodically. We could try to run conda build through this, but IIRC it gobbles up stdout, which is really unhelpful not too mention this would need to go in the CI and thus affect all recipes in the staging area, which I really don't like. If we have to add it to the feedstock as a workaround, that could be ok until we come up with something better. Perhaps, we could try it and see if it helps and just make sure to remove it from the staging area and add the workaround to the feedstock afterwards.

Any other thoughts on this problem? Other potential solutions?

@jakirkham
Copy link
Member

Opened this related issue ( #195 ) for the silent long running Travis CI builds.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 24, 2016

Thanks for the rule of thumb here. Could we have this in the wiki? Honestly copying and pasting it verbatim would be totally fine by me.

@jakirkham I started something here:

https://github.com/conda-forge/staged-recipes/wiki/VC-features

@ocefpaf
Copy link
Member

ocefpaf commented Mar 24, 2016

@groutr and @jakirkham I believe we won't be able to run the tests using Travis-CI's free service . I remember I tried that before and failed. HDF5 testing is pretty intensive and it does takes a lot of time to finish 😒

I have a question for this package and some others in this line up (xerces-c, hdf4, etc). What are the differences from these versions compared to the default channel version? I am sure that there are differences. We should write those in the PR.

I am saying this because I believe we should try to use the default channel as much as possible. We should diverge from the default only one it does not satisfies the needs of the community. (Ideally, if our way is better, the default channel would eventually incorporate our changes.)

Here is the story behind gdal to illustrate what I am saying. I added gdal 1.11.4 because that was not available in the default channel (last 1.x series available is 1.11.2). Note also that fiona was wrongly compiled against gdal 2.0. We needed the conda-forge gdal to have an up-to-date gdal 1.x series and a fully functioning fiona. The coda-forge gdal 1.11.4 package gets its dependencies from the default channel and that seems to work out nicely BTW.

@jakirkham
Copy link
Member

I added gdal 1.11.4 because that was not available in the default channel (last 1.x series available is 1.11.2)....

The story here is the same. This is a newer version of HDF5 than in defaults. I don't know about the other packages as I have not had a chance to look.

nmake
if errorlevel 1 exit 1

REM Install step
Copy link
Member

Choose a reason for hiding this comment

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

Should probably also test here. So, ctest?

@jakirkham
Copy link
Member

I believe we won't be able to run the tests using Travis-CI's free service . I remember I tried that before and failed. HDF5 testing is pretty intensive and it does takes a lot of time to finish 😒

To clarify, the issue we are seeing is not running out of time (it ran ~18mins the upper limit is ~50mins). The issue is that the program is too quiet and so it is terminated. Why it is quiet, I don't know. Though it is worth noting that we are running the same test suite on Circle CI and the whole build takes ~12mins.

Are there some options to make the testing phase nosier? Alternatively, can we run the test suit in pieces? Some solution like this will work around this quiet issue from Travis CI. Worst case maybe we hack travis_wait or similar into the build script. Ugly, but it is important that we verify the packages we ship are in good working order.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 24, 2016

To clarify, the issue we are seeing is not running out of time (it ran ~18mins the upper limit is ~50mins).

You are correct. Maybe we can "hack-patch" the tests to make them noisy.

@jakirkham
Copy link
Member

Instead of wasting your time on this tedious exercise @groutr, I have added a PR ( #199 ) with your changes to try and trick Travis CI into letting us build through by making more noise during build. Nothing else notable has changed.

@jakirkham
Copy link
Member

@ocefpaf, I think I was wrong about the cause of the hang and you were correct. Namely, some HDF5 tests may be to resource intensive for running on CI. It turns out the cache test, in particular, was the culprit here. Fortunately, no other tests suffer. I decided to simply patch the Makefile so that test wouldn't run.

The following PR ( #199 ) includes this one and the tweaks necessary to get the tests to run. Feel free to take a look. Personally, I think we are ready to merge (once CI passes). However, if you have more thoughts, feel free to share.

@pelson pelson merged commit ba73ee7 into conda-forge:master Mar 25, 2016
@jakirkham
Copy link
Member

Thanks @groutr for all your hard work. This has been merged.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 25, 2016

Thanks @groutr and @jakirkham! Nice to have HDF5 in conda-forge!

@jakirkham
Copy link
Member

@ngoldbaum @andrewcollette @ilanschnell @ddale @stuarteberg @FRidh, we are adding hdf5 to conda-forge. This is the community packaging system for use with conda. Packages built here are built with CI and deployed to the conda-forge channel automatically after committing on master of a feedstock (a repo with the recipe plus automatically maintained CI scripts). Eventually, packages submitted here will be included in the defaults channel after some auditing process as mentioned. If there is anything here that you see is off or needs to be fixed, please let me know. If you would like to have access to this feedstock/recipe, I would happily add you to the maintainers list so you can have commit privileges for this recipe. This would be useful for updating the version for instance.

@ngoldbaum
Copy link

Just out of curiosity - why include packages in conda-forge that are in the default channel?

@jakirkham
Copy link
Member

First, the defaults channel will consume packages from conda-forge in the future (within some restrictions that are YTBD). This is better explained in this comment. Second, this is a newer version than the defaults version.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 25, 2016

Just out of curiosity - why include packages in conda-forge that are in the default channel?

That is an important question! The reason conda-forge exists is to augment the default channel with:

  • packages that are not there;
  • newer version of packages that have not yet made into the default channel;
  • play with build switches, optional dependencies, etc;
  • apply patches that are not used in the default channel.

It is important to note that continuum devs are looking closely to conda-forge. That means conda-forge is a fast track to get changes back into the default channel. (See conda-forge/proj.4-feedstock#2 for an example.)

Edit: Instead of sending an e-mail to continuum to ask for changes in a recipe (or a new package) you can just send a PR here. That makes the whole process more transparent and testable because the community will see the recipe changes, how the binaries are build, and will be able to install them before it makes its way to the default channel..

@scopatz
Copy link
Member

scopatz commented Mar 25, 2016

Additionally, as I recall, some default packages do not contain headers and are unsuited to doing development with. Having a separate package is the only way to deal with this. I believe this exact problem came up with HDF5 and Cyclus.

@jakirkham
Copy link
Member

I guess another point is a change merged to a feedstock (repo) here immediately goes to work generating the package. Thus, you could be up and running with your changes quickly after a change is merged. It can also do the build for you on systems that you may not have development infrastructure for (e.g. Windows).

@scopatz
Copy link
Member

scopatz commented Mar 25, 2016

Though @jakirkham - it doesn't seem like the hdf5 windows package has made it to anaconda.org yet

@jakirkham
Copy link
Member

Yes, it was initially working, but it seems to now have some issues that seem related to how feature tracking of VC occurs. Trying to fix in a PR, but the queue is a bit sluggish today.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 25, 2016

Trying to fix in a PR, but the queue is a bit sluggish today.

My bad. I re-started a bunch of failed builds that did not made its way into the channel.

@jakirkham
Copy link
Member

Honestly, I think there is just quite a bit of stuff being added in general so it just adds a bit of load. We can continue tweaking the skip controls.

@jakirkham
Copy link
Member

Though honestly that is preferable to nothing being added and no load. 😄

@ngoldbaum
Copy link

OK, this is interesting. Thanks for the detail! I didn't realize conda-forge packages would eventually make their way into the default channel.

For now I'm going to continue to PR yt releases to conda-recipes, but I will also look into getting conda-forge setup for yt.

@jakirkham
Copy link
Member

OK, this is interesting. Thanks for the detail! I didn't realize conda-forge packages would eventually make their way into the default channel.

Sure, hope that helps. Ask more questions if you want to know more.

For now I'm going to continue to PR yt releases to conda-recipes, but I will also look into getting conda-forge setup for yt.

Definitely, feel free to ping me when you do. Happy to help you get started. To get a rough idea of how that will go there are a couple simple steps.

@mwcraig
Copy link
Contributor

mwcraig commented Mar 25, 2016

If there is a (very out-of-date) recipe for a package currently in conda-recipes do you want me to leave a note there when I add it to conda-forge?

@jakirkham
Copy link
Member

This is a great question @mwcraig and it got me thinking (maybe too much) about many things (maybe too many). Though much if not all of it felt a bit off topic so I have posted my reply in the transition issue. Feedback welcome.

@andrewcollette
Copy link

Also pinging @jreadey; you may want to let your colleagues at the HDF Group know about this in case they want to contribute in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants