Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TMVA] Add support for tf.keras in MethodPyKeras #6568

Merged
merged 3 commits into from Oct 9, 2020
Merged

[TMVA] Add support for tf.keras in MethodPyKeras #6568

merged 3 commits into from Oct 9, 2020

Conversation

lmoneta
Copy link
Member

@lmoneta lmoneta commented Oct 6, 2020

Add support in MethodPyKeras for the Keras version shipped with tensorflow 2 (tf.keras).
At the moment default is still using keras with tensorflow backend

  • tf.keras can be enabled with the option tf.keras=True.
  • If not a keras version >= 2.3 is not found and tensorflow 2 is used then tf.keras is used

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

@lmoneta lmoneta self-assigned this Oct 6, 2020
@lmoneta lmoneta linked an issue Oct 6, 2020 that may be closed by this pull request
[TMVA] Provide support in MethodPyKeras for tensorflow.keras #6553
Closed
@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on mac1015/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@lmoneta
Add support for tf.keras, keras version relased with TensorFlow 2
1e5945c 
The support is provided by adding an option in MethodPyKeras, tf.keras=1
@lmoneta
Fix the initializations in the SetupModel function.
1496a4f 
Move from Init() to SetUpModel() when class state is filled either with options or with XML file

Add a check if Keras version is >=2.3 If it is not and Tensorflow 2 is used then switch automatically to use Tensorflow.Keras
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

stwunsch
stwunsch suggested changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@stwunsch stwunsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think there are some minor issues in the logic. Could you have a look?

tmva/pymva/src/MethodPyKeras.cxx Show resolved Hide resolved
tmva/pymva/inc/TMVA/MethodPyKeras.h Show resolved Hide resolved
tmva/pymva/src/MethodPyKeras.cxx
auto ret = PyRun_String("import keras", Py_single_input, fGlobalNS, fLocalNS);
// need importing also in global namespace
if (ret != nullptr) ret = PyRun_String("import keras", Py_single_input, fGlobalNS, fGlobalNS);
if (ret != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you have two times if(ret != nullptr), you can merge them.

tmva/pymva/src/MethodPyKeras.cxx Show resolved Hide resolved
tmva/pymva/src/MethodPyKeras.cxx Show resolved Hide resolved
tmva/pymva/src/MethodPyKeras.cxx Outdated Show resolved Hide resolved
tmva/pymva/src/MethodPyKeras.cxx Outdated
void MethodPyKeras::Init() {

std::cout << "Init MethodPyKeras " << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this plain cout print or use the TMVA logger

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac1015/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2020-10-08T08:52:49.875Z] axis.obj : fatal error LNK1179: invalid or corrupt file: duplicate COMDAT '??$?8DU?$char_traits@D@std@@@__ROOT@experimental@std@@YA_NV?$basic_string_view@DU?$char_traits@D@std@@@012@0@Z' [C:\build\workspace\root-pullrequests-build\build\hist\histv7\test\histhistv7testUnit.vcxproj]

@lmoneta lmoneta requested a review from stwunsch October 8, 2020 14:15
stwunsch
stwunsch approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@stwunsch stwunsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lmoneta lmoneta merged commit 6aa2f77 into root-project:master Oct 9, 2020
@lmoneta lmoneta mentioned this pull request Oct 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stwunsch stwunsch stwunsch approved these changes

@ashlaban ashlaban Awaiting requested review from ashlaban

@sitongan sitongan Awaiting requested review from sitongan

Assignees

@lmoneta lmoneta

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TMVA] Provide support in MethodPyKeras for tensorflow.keras
3 participants
@lmoneta @phsft-bot @stwunsch