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 License and Requirements to Source Distribtion #1522

Merged
merged 8 commits into from
Jul 5, 2022

Conversation

janjagusch
Copy link
Contributor

Description

Hi!

We're currently making an effort to put the responsible-ai-toolbox on conda-forge. You can find the corresponding PRs here:

We've noticed that there are some files missing in the source distribution that you ship on PyPI.

  • The license is missing
  • The requirements.txt file is missing, which is read inside the setup.py (example)

Adding the requirements.txt to the source distribution can be accomplished by adding a MANIFEST.in file to the root of the package repository. As there is no way to reference files outside the root of the repository when building a source distribution (source), we had to copy the license file into all package directories. We also made sure that the dist/ directory does not get committed to version control when testing this.

Checklist

  • I have added screenshots above for all UI changes.
  • I have added e2e tests for all UI changes.
  • Documentation was updated if it was needed.

@ghost
Copy link

ghost commented Jun 28, 2022

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

Thanks @janjagusch ! This looks fine to me. @xuke444 @imatiach-msft ?

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2022

Codecov Report

Merging #1522 (863df7b) into main (8be72b5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1522   +/-   ##
=======================================
  Coverage   87.27%   87.27%           
=======================================
  Files         108      108           
  Lines        5108     5108           
=======================================
  Hits         4458     4458           
  Misses        650      650           
Flag Coverage Δ
unittests 87.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8be72b5...863df7b. Read the comment docs.

@romanlutz
Copy link
Contributor

FWIW we'll have to override the failing CD build since the secret isn't available for people with their own forks (which is also something we should fix at some point). Since there are no real code changes I'm not worried about that, though.

@imatiach-msft imatiach-msft merged commit 343c925 into microsoft:main Jul 5, 2022
@imatiach-msft
Copy link
Contributor

@janjagusch do you need us to do another pypi release, or are you all set to add those packages to conda-forge?

@imatiach-msft
Copy link
Contributor

imatiach-msft commented Jul 5, 2022

FYI I force-merged this PR due to the CD build failing, I think this is something that we really need to resolve as soon as possible for those who contribute from forks as it's something that has come up numerous times

@janjagusch
Copy link
Contributor Author

@janjagusch do you need us to do another pypi release, or are you all set to add those packages to conda-forge?

Thanks for merging @imatiach-msft! If you could release new versions, that would be great. I can work around the missing files, but this way to build would be much easier. Thanks a lot!

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.

4 participants