-
Notifications
You must be signed in to change notification settings - Fork 0
allowing tensorflow version flexibility when installing from source #7
Conversation
Codecov Report
@@ Coverage Diff @@
## tf250 #7 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 3 -1
Lines 373 371 -2
=========================================
- Hits 373 371 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 implementing this. I was a bit confused about the use of the __init__.py.in
file at first but after building the custom operators I now understand the use. Works well for me.
elif tf.__version__ > __target_tf_version__: # pragma: no cover | ||
raise RuntimeError( | ||
f"qibotf {__version__} requires TensorFlow {__target_tf_version__}." | ||
"Please upgrade qibotf or downgrade TensorFlow.") | ||
" Please upgrade qibotf or downgrade TensorFlow.") |
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.
Aren't these checks equivalent to a single if tf.__version__ != __target_tf_version__
check?
Also regarding the error message, I think there is also the option to just rebuild the custom operators without upgrading/downgrading any of the libraries. However this may be confusing for the average user so it is okay to leave it as it is.
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, indeed, should be equivalent. The if/else is here just to explain to the average user how to solve the incompatibility.
But, I agree that a simple != should make it, e.g. we could say qibotf and tensorflow do not match, please recompile qibotf or check the documentation
. What do you think?
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.
But, I agree that a simple != should make it, e.g. we could say
qibotf and tensorflow do not match, please recompile qibotf or check the documentation
. What do you think?
This sounds good and is a bit more general than the current. I would just add "qibotf and tensorflow versions do not match". I am fine with either message, but perhaps referring to the docs is the best idea.
Following the discussion this morning, here we: