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

split off recipe render from build; recurse to find meta.yaml #908

Merged
merged 11 commits into from May 5, 2016

Conversation

msarahan
Copy link
Contributor

@msarahan msarahan commented Apr 29, 2016

Closes #887

This is the start of splitting off recipe rendering from building. I am trying to avoid duplicating code, so I'm splitting the existing code off into other modules.

Right now, it only prints the processed metadata, not a yaml file. That's easy enough to write as an addon option later - I'm expecting that the raw dictionary will be the most useful thing to people, rather than going back to some file on disk. My idea is that the build module will just call functions from this file to render each recipe's meta.yaml.

This works right now, with a trivial test of a recipe from conda-forge (zlib). Original recipe at https://github.com/conda-forge/zlib-feedstock, which I have cloned as a submodule into the packages/zlib folder:

$ conda render packages/zlib
{u'about': {u'home': u'http://zlib.net/',
            u'license': u'zlib (http://zlib.net/zlib_license.html)',
            u'license_family': u'Other',
            u'license_file': u'license.txt',
            u'summary': u'Massively spiffy yet delicately unobtrusive compression library'},
 u'build': {u'features': u''},
 u'extra': {u'recipe-maintainers': [u'groutr', u'msarahan', u'ocefpaf']},
 u'package': {u'name': u'zlib', u'version': u'1.2.8'},
 u'requirements': {u'build': [u'cmake']},
 u'source': {u'fn': u'zlib-1.2.8.tar.gz',
             u'sha256': u'36658cb768a54c1d4dec43c3116c27ed893e88b02ecfcb44f2166f9c0b7f2a0d',
             u'url': u'http://zlib.net/zlib-1.2.8.tar.gz'},
 u'test': {u'commands': [u'test -f ${PREFIX}/lib/libz.a']}}

This change also makes conda-build recursively search for meta.yaml in any folder you provide, so it should make conda build more directly compatible with the conda-forge layout of having the recipe one level deeper.

I'll be cleaning this up and coming up with test cases - please kick the tires and point out any details I should pay special attention to.

CC @jakirkham @stuarteberg @kalefranz @pelson @groutr

@msarahan msarahan added the in_progress [deprecated] use milestones/project boards label Apr 29, 2016
@msarahan msarahan changed the title WIP: start splitting off recipe render from build; recurse to find meta.yaml split off recipe render from build; recurse to find meta.yaml Apr 30, 2016
@msarahan msarahan added the 4_Needs_Review [deprecated] use milestones/project boards label Apr 30, 2016
@msarahan
Copy link
Contributor Author

msarahan commented May 1, 2016

This now also renders YAML, keeping the keys in the order we expect:

$ conda render ~/Documents/code/anaconda/packages/zlib --yaml
package:
    name: zlib
    version: 1.2.8
source:
    fn: zlib-1.2.8.tar.gz
    sha256: 36658cb768a54c1d4dec43c3116c27ed893e88b02ecfcb44f2166f9c0b7f2a0d
    url: http://zlib.net/zlib-1.2.8.tar.gz
build:
    features: ''
requirements:
    build:
        - cmake
test:
    commands:
        - test -f ${PREFIX}/lib/libz.a
extra:
    recipe-maintainers:
        - groutr
        - msarahan
        - ocefpaf

@jakirkham
Copy link
Member

Could we tweak it to space out the main sections?

@jakirkham
Copy link
Member

Also, just to clarify, will this proceed through the download (but not build phase)? What is the option to enable/disable this in case people don't want to proceed that far?

@msarahan
Copy link
Contributor Author

msarahan commented May 1, 2016

By default, it will not download the source. You can pass the -s/--source option to do so. If it can't fill in fields without the source, it will throw errors (Jinja strict undefined stuff that @stuarteberg came up with).

You can also pass the -o/--output flag to print the output filename (as conda build did/does), rather than the meta.yaml data.

Sorry, but adding spaces is not something I want to fight PyYAML to do. It was enough fun getting it to where it is now. If you want to tackle that, I think the right place would be https://github.com/conda/conda-build/pull/908/files#diff-154513cee3e5765ecebd315978958f40R212

@stuarteberg
Copy link
Contributor

Looks good to me, but I have a couple of questions:

conda build --output first attempts parse meta.yaml without downloading the source, but then it automatically downloads the source if necessary. But as you noted above, the new render command doesn't do that:

By default, it will not download the source. You can pass the -s/--source option to do so. If it can't fill in fields without the source, it will throw errors

So in other words, the user is expected to interpret the error and add --source when necessary? I can see why we wouldn't want to preemptively download the source (it won't be necessary for all recipes), but in the case of an error, isn't it nicer to just go ahead and give it a try on the user's behalf, as we do for conda build --output?

Also, I have an unrelated question about the output format. (If this PR is still WIP, then this question may be premature...)

The current version of your PR writes the fully-rendered YAML to stdout. Are you planning to change that before this PR is merged? It seems like a file output would be more desirable because misc. functions throughout the conda code base write various status messages to stdout, which will pollute the output with non-YAML junk.

@msarahan
Copy link
Contributor Author

msarahan commented May 2, 2016

@stuarteberg all good ideas. This is why I ping you. I also had the thought that there need not be a --yaml option - who wants output in any other format? If they want the metadata itself as a python object, they can call the render_recipe function themselves. I will replace it with a -f/--file option that writes to a file (the argument to -f/--file), but keep the default as writing to stdout. Sound good?

As for downloading the source: you're right. I like the transparent failover better than assuming interpretation of error messages.

@stuarteberg
Copy link
Contributor

Sounds good!

@codecov-io
Copy link

Current coverage is 100%

Merging #908 into master will decrease coverage by +90.1%

@@            master   #908   diff @@
=====================================
  Files           39      0     -39   
  Lines         5136      0   -5136   
  Methods          0      0           
  Messages         0      0           
  Branches         0      0           
=====================================
- Hits           509      0    -509   
+ Misses        4627      0   -4627   
  Partials         0      0           
  1. 36 files (not in diff) in conda_build were deleted. more

Powered by Codecov. Last updated by 2db31bb...a67b79d

@jakirkham
Copy link
Member

I think I missed the recurse portion when skimming this quickly before. Would this be useful for feedstocks?

@msarahan
Copy link
Contributor Author

msarahan commented May 3, 2016

@jakirkham Yes, that was my motivation. I can now replace things in the main repo with submodules to feedstocks. Closing the loop, little by little.

I still need to tie in rendering the recipe for tools that are expecting static content, but it's progress.

@stuarteberg
Copy link
Contributor

This change also makes conda-build recursively search for meta.yaml in any folder you provide

I've been contemplating a kinda similar feature, but haven't taken the time for a PR yet: What do you guys think about a RECIPE_PATH environment variable that conda-build uses to search for recipes it can't find?

RECIPE_PATH=/path/to/some-dir:/path/to/some-other-dir conda build mypackage

... if that's off-topic I can raise it in a different PR.

@jakirkham
Copy link
Member

I love it. Sorry, I got distracted by the static rendering. Lots of great stuff in this PR. 👍

I can now replace things in the main repo with submodules to feedstocks. Closing the loop, little by little.

👍 Do you want help doing this after the release? It sounds like we are entering phase 3.

cc @pelson

I still need to tie in rendering the recipe for tools that are expecting static content, but it's progress.

Are you meaning like a public API or are there other tools that you guys have that will need this?

@msarahan
Copy link
Contributor Author

msarahan commented May 3, 2016

@stuarteberg That's not a bad idea. I don't think I would use it, but I'm sure others would. I have many folders of recipes, with very high degrees of overlap. Having this feature would confuse me a lot on which collection I was getting. I do think it deserves its own PR.

@msarahan
Copy link
Contributor Author

msarahan commented May 3, 2016

@jakirkham

Do you want help doing this after the release? It sounds like we are entering phase 3.

Help removing recipes from conda-recipes would be great. I will ask again about the CLA - if it would be necessary for this. Internal people have to do the internal repo, obviously.

Are you meaning like a public API or are there other tools that you guys have that will need this?

Most just the legacy internal build system. It does a lot of moving packages around and examining metadata, so I need to make sure that wherever it looks, it finds fully rendered recipes.

@jakirkham
Copy link
Member

jakirkham commented May 3, 2016

Help removing recipes from conda-recipes would be great. I will ask again about the CLA - if it would be necessary for this. Internal people have to do the internal repo, obviously.

I mean if you guys would rather merge PRs that is fine to. I'm not picky here. Whatever works really. I'm just asking so as to know how we and others at conda-forge should start to act in this regard.

Most just the legacy internal build system. It does a lot of moving packages around and examining metadata, so I need to make sure that wherever it looks, it finds fully rendered recipes.

I see. Thanks for clarifying.

@jakirkham
Copy link
Member

So, I'm thinking this might be part of our solution to automating recipe generation to cut down time spent on minutiae. See this comment for some thoughts.

@jakirkham
Copy link
Member

jakirkham commented May 3, 2016

So, I tried to build this example package, but it doesn't quite work as it doesn't like conda.yaml. So, I renamed the file to meta.yaml and was able to build it with conda build. However, I can't get conda render to work. Basically, this is complaining there is no source defined though I'm sure it will complain about others.

@msarahan
Copy link
Contributor Author

msarahan commented May 3, 2016

so the issue with conda render is that it expects the few required fields too early - before the first template pass. I'm going to see about changing that.

@msarahan
Copy link
Contributor Author

msarahan commented May 3, 2016

@jakirkham should work better now.

@jakirkham
Copy link
Member

Thanks. I'll give it another try.

@jakirkham
Copy link
Member

Works. Thanks @msarahan.

package:
    name: test_package
    version: 1.1.0
build:
    entry_points: ''
    number: '1'
    script:
        - cd $RECIPE_DIR
        - $PYTHON setup.py install --single-version-externally-managed --record=record.txt
requirements:
    build:
        - python
        - setuptools
    run:
        - python
        - Flask
        - werkzeug
test:
    imports: ''
    requires: ''

@jakirkham
Copy link
Member

So, I tried basically using this on the setup.py from pandas, but it got a little picky about having things like pytz >=2011k instead of pytz >= 2011k. Now we can and probably should clean these sorts of whitespace things in our rendering process (I added my own hack for now), but maybe it could be a little more relaxed about this sort of stuff for rendering as it can be tricky to catch all of that stuff on the first pass and it can be cleaned up right after.

@jakirkham
Copy link
Member

Kind of weird that the build number keeps getting turned into a string. Not sure where that is coming from.

@jakirkham
Copy link
Member

When you are doing this rendering @msarahan, are you including the about section? It seems to be missing in many cases.

# Next bit of stuff is to support YAML output in the order we expect.
# http://stackoverflow.com/a/17310199/1170370
class MetaYaml(dict):
fields = ["package", "source", "build", "requirements", "test", "extra"]
Copy link
Member

Choose a reason for hiding this comment

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

Could we please add about here?

Copy link
Member

Choose a reason for hiding this comment

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

Should come after test, but before extra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Oversight. Will add.

Copy link
Member

@jakirkham jakirkham May 3, 2016

Choose a reason for hiding this comment

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

No worries. With this change, things appear to work for me.

@jakirkham
Copy link
Member

When you are doing this rendering @msarahan, are you including the about section? It seems to be missing in many cases.

Nevermind. Answered my own question. It would be great if we could add about here.

@jakirkham
Copy link
Member

jakirkham commented May 3, 2016

So, I made this variant based on the included template, which looks like this still kind of WIP, but it is coming along nicely. From that, I can generate the pandas recipe as long as it is included in the source directory (next to setup.py).

Would be good to figure out how we can work this into more of a conda skeleton pypi operation. That way, we can get checksums, fill out the download url, and such.

Also, we could probably get the recipe-maintainers filled out with a user's handle given a little more detective work.

The license unfortunately is always a mess as no one seems to think it worthwhile to specify the license version in the license field. We might be able to get around this by using classifiers if they added one specifying the license version. Though, in the case of pandas, they don't. Maybe this is the tedious step we do manually. I guess it is not so bad as we really don't want to get this wrong and we have automated most everything else.

@jakirkham
Copy link
Member

Added this issue ( #914 ) to try and refactor conda skeleton and expose that functionality to meta.yaml in a straightforward manner. We could push the existing approach the rest of the way with this change I think.

@msarahan
Copy link
Contributor Author

msarahan commented May 5, 2016

Discussed with @kalefranz. Long-term, need to move logic out of CLI files (main_*). OK for now, though. Merging.

@github-actions
Copy link

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to generate static meta.yaml file(s)
4 participants