-
Notifications
You must be signed in to change notification settings - Fork 458
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
feat: Adds conda recipe & corresponding CI jobs #1414
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1414 +/- ##
==========================================
- Coverage 95.79% 95.78% -0.02%
==========================================
Files 155 155
Lines 6950 6950
==========================================
- Hits 6658 6657 -1
- Misses 292 293 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Overall fine but i would suggest to update the docs accordingly :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again 👍
Last two comments from my side ^^
VERSION: ${{ steps.release_tag.outputs.VERSION }} | ||
run: | | ||
echo "BUILD_VERSION=${VERSION}" >> $GITHUB_ENV | ||
python setup.py sdist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling setup.py
directly is deprecated, you'll want something like python -m build --sdist
instead
see also https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frgfm ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know, thanks mara!
But for this PR, I don't think we need the edit:
- we'd have to do this for the other occurrences in the repo (feels like a standalone PR to me, and we should check backward compatibility)
- it will be deprecated but it's still working to the best of my understanding right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be deprecated but it's still working to the best of my understanding right?
Yes, for the time being. They might want to remove it after some deprecation period, but I don't know if they'll ever do, as this somehow doesn't seem to have reached the crowd at large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @frgfm 🤗
@odulcy-mindee How does it look do we have a token now ? 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token has been added !
I suppose you're aware the installation instructions are yet TBD |
@mara004 yep after the next release if it's available :) For the moment CI conda build job fails this needs to be fixed first ^^ |
This PR introduces the following modifications:
Note: for now the recipe is without TF or PyTorch. Conditional builds seem tricky for now 😅
cc @mara004
Closes #113