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

Adding Scalene. #18747

Merged
merged 38 commits into from
Aug 24, 2022
Merged

Adding Scalene. #18747

merged 38 commits into from
Aug 24, 2022

Conversation

emeryberger
Copy link
Contributor

@emeryberger emeryberger commented Apr 24, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

(I have self listed myself in the maintainer section)

Fixes #17059
Closes #17325

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found some lint.

Here's what I've got...

For recipes/scalene:

  • The requirements/ sections should be defined in the following order: build, host, run; instead saw: host, build, run.
  • There are too few lines. There should be one empty line at the end of the file.
  • Recipe maintainer "Emery Berger" does not exist
  • If python is a host requirement, it should be a run requirement.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found it was in an excellent condition.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks @emeryberger! 🙏

Had a few suggestion below

recipes/scalene/meta.yaml Show resolved Hide resolved
recipes/scalene/meta.yaml Outdated Show resolved Hide resolved
recipes/scalene/meta.yaml Outdated Show resolved Hide resolved
recipes/scalene/meta.yaml Show resolved Hide resolved
recipes/scalene/meta.yaml Outdated Show resolved Hide resolved
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found some lint.

Here's what I've got...

For recipes/scalene:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

emeryberger and others added 2 commits August 6, 2022 17:45
Co-authored-by: jakirkham <jakirkham@gmail.com>
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found it was in an excellent condition.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found some lint.

Here's what I've got...

For recipes/scalene:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [43, 44]

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found it was in an excellent condition.

@emeryberger
Copy link
Contributor Author

@jakirkham Thanks for your help. Finally a successful run on all supported platforms, and I believe I have addressed all concerns.

@nicovank
Copy link

@conda-forge/help-python ready for review

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found some lint.

Here's what I've got...

For recipes/scalene:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/scalene) and found it was in an excellent condition.

@emeryberger
Copy link
Contributor Author

Co-authored-by: jakirkham <jakirkham@gmail.com>
@jakirkham jakirkham requested a review from ocefpaf August 23, 2022 18:44
Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

It's still unclear to me whether scalene.old is intended to be packaged. This module does not have an __init__.py If this module is not part of the package, then it should be excluded and numpy is not a run dependency.

@emeryberger
Copy link
Contributor Author

@carterbox scalene.old is no longer in the PR (unless I am missing something).

@jakirkham
Copy link
Member

Yeah I don't see this either. Could you please clarify what you are referring to Daniel?

@emeryberger
Copy link
Contributor Author

I see, you mean the old/ in the scalene source directory. I'm not sure why minutiae about the contents of the source repo matter for the purposes of this package... in any event, that code is not a module, but a currently-disabled piece of functionality from Scalene's leak analysis algorithm.

@jakirkham
Copy link
Member

Thanks for the context Daniel! 🙏

Given this is not unique to the Conda package (both the sdist and whls include it), would suggest we leave it alone (instead of doing something special/different for the Conda package).

If we want this addressed, think it would be best to raise a new issue upstream and suggest excluding these files via the MANIFEST.in. That way they would be removed from all redistributables (including Conda packages).

@emeryberger
Copy link
Contributor Author

Thanks for the suggestion re: the MANIFEST.in file, @jakirkham - I've made that modification (excluding old/) and it will be reflected in the next release.

@jakirkham
Copy link
Member

Thanks Emery! 🙏

For reference, that change is here ( plasma-umass/scalene@482284c )

@carterbox
Copy link
Member

I'm not sure why minutiae about the contents of the source repo matter for the purposes of this package

It's related to packaging because it's the only place that numpy is imported. Since numpy is not listed in setup.py, you are adding an extra dependency for conda users.

@emeryberger
Copy link
Contributor Author

OK - that said, that file is never included anywhere, so I don't understand why it's an extra dependency (it's currently orphaned). In any event, I'd hate to cut a new release to just exclude this one file, but let me know if I need to. Thanks.

Copy link
Contributor Author

@emeryberger emeryberger left a comment

Choose a reason for hiding this comment

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

(No changes to this file.)

@carterbox
Copy link
Member

scalene.old will no longer be shipped with some unknown future release of setuptools or you can manually exclude it from the next release.

@carterbox carterbox merged commit aaaba6e into conda-forge:main Aug 24, 2022
@h-vetinari
Copy link
Member

Woohoo! 🥳

@jakirkham
Copy link
Member

Thanks all! 🙏

@emeryberger
Copy link
Contributor Author

Thanks, everyone, for all the help in this process! I expect our next package to integrate more smoothly :).

Quick question: what do I need to do get a "osx-arm64" build?

@BastianZim
Copy link
Member

Just add the name here and a bot will make a PR: https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/main/recipe/migrations/osx_arm64.txt

@emeryberger
Copy link
Contributor Author

Hi all @jakirkham @carterbox - I'd like to support Windows users with conda but I don't know how (the Windows version doesn't require a build). Any help would be most appreciated! (See https://github.com/conda-forge/scalene-feedstock)

@carterbox
Copy link
Member

Please open an issue on your feedstock for this discussion, so that this conversation doesn't get lost. We get 10s of notifications from staged recipes per day.

@jakirkham
Copy link
Member

xref: conda-forge/scalene-feedstock#10

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

Successfully merging this pull request may close these issues.

Package scalene
8 participants