-
Notifications
You must be signed in to change notification settings - Fork 0
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
recbole 1.2.0 ❄️ #1
Conversation
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.
LGTM pending a successful build upon removing the staging channels in abs.yaml.
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.
Couple of minor comments but looks good
|
||
build: | ||
number: 0 | ||
# s390x: kmeans-pytorch (numba) unavailable | ||
# py>=312: ray unavailable |
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.
Do you want to build ray so you can get 3.12? I suppose this depends on whether snowflake wants to pay for that.
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.
@danpetry we are on hold for this one (see ticket). We do have ray in our pipeline, if it's out before we get the OK for this one then I will remove the skip
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.
ray-tune
does not have yet py==3.12
variant.
ray-tune 2.6.3 py310hca03da5_2 pkgs/main
ray-tune 2.6.3 py311hca03da5_2 pkgs/main
ray-tune 2.6.3 py38hca03da5_2 pkgs/main
ray-tune 2.6.3 py39hca03da5_2 pkgs/main
skipping py==312
because this ticket was on hold waiting for feedback for too long already.
# originally ==0.2.5 adapted with a patch | ||
# to work with the more recent version we | ||
# have (0.2.7) | ||
- hyperopt >=0.2.5 |
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.
should it be run_constrained? It's an extras_require
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 is,
the suggested smoke test from upstream uses it, though (see my comment in the issue here: RUCAIBox/RecBole#2105).
unfortunately, I did not receive any answer also for the rest of the seemingly optional dependencies (see comment a few lines before, and also in the above issue link).
Therefore, I moved all of those optional deps except some that we do not have to run
.
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.
recbole
upstream replied that the following dependencies are optional:
torch-scatter
,dgl
andkmeans-pytorch
are all optional.
ref: RUCAIBox/RecBole#2105 (comment)
Therefore, I am moving the optional dependencies in run_constrained
.
recbole 1.2.0 ❄️
Destination channel: Snowflake
Links
Depends on
Explanation of changes:
kmeans-pytorch
not mentioned in the setup.py, open an issue about it here: [🐛BUG] Mandatory and optional dependencies RUCAIBox/RecBole#2105colorlog
pinning relaxed to lower bound instead of exact. We have a more recent version and it seems to work (see screenshot).hyperopt
in a more recent version than specified in upstream (see patch comment).