-
Notifications
You must be signed in to change notification settings - Fork 445
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
[CI] update CI actions #1558
[CI] update CI actions #1558
Conversation
@odulcy-mindee for the PR it's fine but could you check afterwards that the |
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@v3 | ||
with: | ||
flags: unittests | ||
fail_ci_if_error: true | ||
token: ${{ secrets.CODECOV_TOKEN }} |
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.
This secret does not exist. What should I do to get one ?
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.
@odulcy-mindee login at https://about.codecov.io/ and copy the token (repos -> doctr -> settings) the sec token is it which needs to be added to the repo secrets :)
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.
done
pyproject.toml
Outdated
@@ -38,7 +38,7 @@ dependencies = [ | |||
"numpy>=1.16.0,<2.0.0", | |||
"scipy>=1.4.0,<2.0.0", | |||
"h5py>=3.1.0,<4.0.0", | |||
"opencv-python>=4.5.0,<5.0.0", | |||
"opencv-python-headless>=4.5.0,<5.0.0", |
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.
This change would require a little more thought. Here, it means that we can no longer use the view methods otherwise it will generate an error for users and they will have to reinstall/overwrite the current installation with opencv-python
.
I'm more in favour of keeping the non-headless package for now and find a way to have headless by default and have an optional dependency so it's not opencv-python-headless
but opencv-python
.
It's harder than having just an optional dependancy: it should replace opencv-python-headless
by the other one and I'm not sure we can do that with pyproject.toml
.
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.
This change would require a little more thought. Here, it means that we can no longer use the view methods otherwise it will generate an error for users and they will have to reinstall/overwrite the current installation with
opencv-python
.I'm more in favour of keeping the non-headless package for now and find a way to have headless by default and have an optional dependency so it's not
opencv-python-headless
butopencv-python
. It's harder than having just an optional dependancy: it should replaceopencv-python-headless
by the other one and I'm not sure we can do that withpyproject.toml
.
you mean the visualization we use at doctr ? That's done with matplotlib so doctr itself doesn't requires opencv GUI support :)
So on our end we reduce it only because if a user has already installed python-doctr it will still work with python-doctr-headless (let me shortly check about the installation)
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.
ok tested and the behaviour is strange so yeah you are right would require something like python-doctr[headless]
Switch PR to only CI updates :)
This PR: