-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
TensorFlow 2.6 upgrade #1565
TensorFlow 2.6 upgrade #1565
Changes from 49 commits
66e5d3c
3396f2c
153099e
d67955e
b572bbe
e8bc100
68af516
aafbe1a
899b43a
cb60a2f
9f39411
edf9b4c
a98750b
10cd5a7
e1560f9
d2b03b7
2efcac0
e5ec621
f3d0437
ba702e5
1b83ffe
d258bfe
1d1b211
f205d15
df7ed26
c29bc41
de2b3f7
f5d28e4
b6ceeec
68979f1
06c93ba
0e57c04
26eb236
2fd0726
6f1d85e
774c8eb
93fb27b
1d94f13
8503c51
a743eba
7d36296
d5491b7
f325a2c
11e4c86
a8e316d
1f9c24a
bd32d2f
fde38fb
679d50c
7688dc1
2523447
3ef366a
ecff395
dd0b29e
8f32dda
23ae6e0
55a8199
383c475
5c18fb6
dfc8eac
1dc5d5c
292f139
2bce47a
2eb8f93
bfbb4b0
3eae4b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,20 +62,28 @@ | |
}, | ||
{ | ||
"cell_type": "code", | ||
"execution_count": 2, | ||
"execution_count": 1, | ||
"metadata": { | ||
"pycharm": { | ||
"is_executing": false | ||
} | ||
}, | ||
"outputs": [ | ||
{ | ||
"name": "stderr", | ||
"output_type": "stream", | ||
"text": [ | ||
"/anaconda/envs/tf2/lib/python3.7/site-packages/papermill/iorw.py:50: FutureWarning: pyarrow.HadoopFileSystem is deprecated as of 2.0.0, please use pyarrow.fs.HadoopFileSystem instead.\n", | ||
" from pyarrow import HadoopFileSystem\n" | ||
] | ||
}, | ||
Comment on lines
+72
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this error is weird, it seems that it comes from papermill but we don't import papermill on this notebook. I see this appears in all notebooks, it would be good to clear this error. This can be done by executing twice this cell or by upgrading the papermill version to one that has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually this cell imports scrapbook, which imports papermill. The versions of papermill and scrapbook are the latest ones. I think the only thing we can do is wait for scrapbook to change this in a later version. |
||
{ | ||
"name": "stdout", | ||
"output_type": "stream", | ||
"text": [ | ||
"System version: 3.6.11 | packaged by conda-forge | (default, Aug 5 2020, 20:09:42) \n", | ||
"System version: 3.7.11 (default, Jul 27 2021, 14:32:16) \n", | ||
"[GCC 7.5.0]\n", | ||
"Tensorflow version: 1.15.2\n" | ||
"Tensorflow version: 2.6.1\n" | ||
miguelgfierro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
] | ||
} | ||
], | ||
|
@@ -359,10 +367,12 @@ | |
], | ||
"metadata": { | ||
"celltoolbar": "Tags", | ||
"interpreter": { | ||
"hash": "3a9a0c422ff9f08d62211b9648017c63b0a26d2c935edc37ebb8453675d13bb5" | ||
}, | ||
Comment on lines
+370
to
+372
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this hash? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. It looks like metadata added by VSCode, maybe it identifies the Python interpreter VSCode uses. |
||
"kernelspec": { | ||
"display_name": "Python (reco_gpu)", | ||
"language": "python", | ||
"name": "reco_gpu" | ||
"display_name": "Python 3.7.11 64-bit ('tf2': conda)", | ||
"name": "python3" | ||
}, | ||
"language_info": { | ||
"codemirror_mode": { | ||
|
@@ -374,7 +384,7 @@ | |
"name": "python", | ||
"nbconvert_exporter": "python", | ||
"pygments_lexer": "ipython3", | ||
"version": "3.6.11" | ||
"version": "3.7.11" | ||
}, | ||
"pycharm": { | ||
"stem_cell": { | ||
|
@@ -388,4 +398,4 @@ | |
}, | ||
"nbformat": 4, | ||
"nbformat_minor": 2 | ||
} | ||
} |
Large diffs are not rendered by default.
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.
--no-cache --no-binary scikit-surprise
how can we remove these from the installation? before we didn't have it, we do we need it 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.
Yes, we need this because we want pip to build Surprise from the source (I changed
setup.py
in an earlier PR to link to the source on GitHub). Otherwise we get the conflict between Surprise and Numpy 1.19 (array size mismatch).Maybe there is a way to do this inside
setup.py
, any ideas?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.
@NicolasHug hey Nicolas, we are upgrading to TF 2 in recommenders, we are having a conflict between surprise and TF 2.
What @anargyri is doing at the moment is adding the tarball and also we have to install
--no-cache --no-binary scikit-surprise
before recommenders.Do you know what is the best way to install surprise with TF2 and add it in the
extra_requires
like we had before?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.
@miguelgfierro See this issue and discussion
@NicolasHug one possible alternative would be to publish another version of Surprise on PyPI that has .pyx files compiled with numpy version 1.19.
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.
Hi all, sorry for the troubles - any chance you could just rely on conda-forge to install surprise binaries directly instead of building from source?
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, we'd really appreciate it 🙏
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 for your support @NicolasHug!
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.
I had time to look into this a little. I'm a bit surprised that we're getting this "size changed" issue. According to numpy/numpy#18709, this issue happens when one is trying to run code that was compiled against a more recent numpy version than what is currently installed.
But since I released surprise 1.1.1 in July 2020, the latest possible version I could have possibly used is numpy 1.19 (which was released one month earlier).
So I'm not quite sure why you're getting this error with 1.19 TBH. Is it possible that you're installing / building surprise with e.g. numpy 1.20/1.21 and then downgrade to numpy 1.19? If that's the case, then the error is expected.
On my end, I'm only able to reproduce the issue if I build on (e.g.) 1.20 and then try to import surprise with 1.19. If I build with 1.17, I can import surprise with all versions all the way up to 1.21.
Anyway, it's possible that I'm missing something. Since 1.17 seems to work fine upwards (and since it's older and thus less constraining than 1.19 in terms of dependency satisfaction), I uploaded a new release where Cython files were compiled against 1.17. I only updated it to the pypi-test channel for now. Before I push it for good on the official PyPI channel, could you please check that it actually works for you guys? You can install from pypi-test with e.g.
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple scikit-surprise
Note: when trying that locally you might need a
--no-cache-dir
flag to make sure stuff gets re-compiled when you switch numpy versions.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 @NicolasHug, I still get the same behavior with the new release, however. As before, pip installing without the no-binary option leads to the error, using this option works fine.
According to the issue you linked, it seems that others have been affected by this too. It looks like this behavior is not really about the version of numpy itself but has to do with the Cython compilation. So that everything works when
python setup.py
is run during the pip installation (which is the effect of the no-binary option). Gensim has observed the same.Maybe we should wait until there is a fix by numpy or cython. Until then, we could require users to use
--no-binary
. Thanks for trying anyway!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.
@NicolasHug @miguelgfierro I did some further investigation. To summarize,
installs but then importing surprise raises ValueError.
works correctly.
So there is a workaround for people who want to use numpy <=1.19 along with surprise.
In our case, it still doesn't work via our
setup.py
(unless you pre-install cython), but this must be some pip / setuptools bug.