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 DeepEcho #16707

Merged
merged 12 commits into from
Nov 19, 2021
Merged

Adding DeepEcho #16707

merged 12 commits into from
Nov 19, 2021

Conversation

amontanez24
Copy link
Contributor

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.

@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 (deepecho) and found some lint.

Here's what I've got...

For deepecho:

  • Please do not delete the example recipe found in recipes/example/meta.yaml.

@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/deepecho) and found it was in an excellent condition.


extra:
recipe-maintainers:
- conda-forge/rdt
Copy link
Contributor Author

@amontanez24 amontanez24 Nov 2, 2021

Choose a reason for hiding this comment

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

I want to assign our team to multiple conda-forge projects. I saw this is the suggested way here (https://conda-forge.org/docs/user/faq.html#faq-teams), but I was wondering if there is a way to change the name of our team to sdv-dev?

@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/deepecho) and found some lint.

Here's what I've got...

For recipes/deepecho:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [26, 27]
  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@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/deepecho) and found some lint.

Here's what I've got...

For recipes/deepecho:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@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/deepecho) and found some lint.

Here's what I've got...

For recipes/deepecho:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

@amontanez24
Copy link
Contributor Author

@conda-forge/help-python
This package depends on pytorch, which doesn't support windows on conda-forge (https://anaconda.org/conda-forge/pytorch), causing the tests to fail. I tried to switch to only installing it for non windows environments, and installing pytorch-cpu for windows instead by using selectors, but then I got the noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python. lint error. I then tried making the package non noarch, but then I get the Non noarch packages should have python requirement without any version constraints. lint error.

Our package only runs on python 3.6-3.8 at the moment so we need version constraints. What is the best way to get around the pytorch windows issue?

@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/deepecho) and found some lint.

Here's what I've got...

For recipes/deepecho:

  • Non noarch packages should have python requirement without any version constraints.

@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/deepecho) 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/deepecho) and found some lint.

Here's what I've got...

For recipes/deepecho:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@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/deepecho) and found it was in an excellent condition.

@BastianZim
Copy link
Member

BastianZim commented Nov 9, 2021

There are two ways to solve the windows PyTorch issue:

  1. Don't offer a windows version. This means keeping the package noarch and just ignoring the missing PyTorch error. When your package is then being downloaded the conda-solver will inform the user that a dependency is missing and fail. This is expected and acceptable. Once PyTorch is available on Windows, the user will then be able to install the package.
  2. Don't be noarch anymore. In that case, the python version restrictions can be implemented using skip: https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#skipping-builds and the selectors are here: https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#preprocessing-selectors

As a note though, python3.6 is not supported on conda-forge anymore, you're package will therefore only be built for python3.7 and python3.8.

Option 1. is easier to implement and maintain but it means that no one on Windows will be able to install your package until PyTorch is available. If that is necessary, you'll have to go with option 2. I would also suggest following conda-forge/pytorch-cpu-feedstock#32 as that tracks windows support.

@amontanez24
Copy link
Contributor Author

There are two ways to solve the windows PyTorch issue:

  1. Don't offer a windows version. This means keeping the package noarch and just ignoring the missing PyTorch error. When your package is then being downloaded the conda-solver will inform the user that a dependency is missing and fail. This is expected and acceptable. Once PyTorch is available on Windows, the user will then be able to install the package.
  2. Don't be noarch anymore. In that case, the python version restrictions can be implemented using skip: https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#skipping-builds and the selectors are here: https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#preprocessing-selectors

As a note though, python3.6 is not supported on conda-forge anymore, you're package will therefore only be built for python3.7 and python3.8.

Option 1. is easier to implement and maintain but it means that no one on Windows will be able to install your package until PyTorch is available. If that is necessary, you'll have to go with option 2. I would also suggest following conda-forge/pytorch-cpu-feedstock#32 as that tracks windows support.

I think we'd like to go with option 1. Is there anything else that would need to be done to make that work?
The osx build failing is puzzling because it passed before and the error seems to have to do with the importing of a package called jmp (https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_apis/build/builds/402443/logs/25).
https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=402443&view=logs&j=e35d4f76-8ff2-5536-d795-df91e63eb9f7&t=fa7b4b17-b6ff-5c9c-8cfc-15f888c92310&l=2585

I would just restart the workflow but I don't have permission.

We also had a question about using the same maintainers as another library we maintain. Are we good to just put conda-forge/rdt and is there anyway to change that name?

@BastianZim
Copy link
Member

Before I have a look at the rest of the recipe:

I would just restart the workflow but I don't have permission.

You can just close and reopen the PR.

We also had a question about using the same maintainers as another library we maintain. Are we good to just put conda-forge/rdt and is there anyway to change that name?

Yeah I saw it but didn't answer that because I'm not 100% sure. :)
These teams are created for a feedstock so the team will probably always have the name of the feedstock. Therefore, I'd say no but a reviewer from core might be able to answer that better.
You're good though with just adding the conda-forge/rdt team as long as you follow https://conda-forge.org/docs/user/faq.html#faq-teams

Copy link
Member

@BastianZim BastianZim left a comment

Choose a reason for hiding this comment

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

I have added two small cosmetic comments. These aren't necessary and you'll have to double-check that I linked to the correct website but it might be nice to inform your users of your websites.

- pip

about:
home: https://github.com/sdv-dev/DeepEcho
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
home: https://github.com/sdv-dev/DeepEcho
home: https://sdv.dev

recipes/deepecho/meta.yaml Show resolved Hide resolved
@BastianZim
Copy link
Member

You can ignore the jmp error, that looks like a conda-forge bug.

Copy link
Member

@BastianZim BastianZim left a comment

Choose a reason for hiding this comment

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

Since you distribute your tests through PyPI, you can also add them here. This is not necessary but might make it easier to catch conda-forge related errors.

recipes/deepecho/meta.yaml Show resolved Hide resolved
recipes/deepecho/meta.yaml Show resolved Hide resolved
@BastianZim
Copy link
Member

If you’re otherwise ready, feel free to ping the review team as outlined in the PR description when you edit it (I‘m just a normal user so I can’t merge it).

@amontanez24
Copy link
Contributor Author

@conda-forge/staged-recipes This Pr should be ready for review

@amontanez24
Copy link
Contributor Author

@conda-forge-admin, please ping team
This PR is ready, it just fails because torch has not put up the latest version on conda-forge for windows

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@ocefpaf ocefpaf merged commit 22270e0 into conda-forge:main Nov 19, 2021
@gonzalocasas gonzalocasas mentioned this pull request Feb 18, 2024
10 tasks
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.

4 participants