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.

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

Preparing sympy for tensorflow release #17103

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@czgdp1807
Copy link
Member

commented Jun 26, 2019

References to other Issues or PRs

Fixes #17105

Brief description of what is fixed or changed

I noticed that the master build is failing due to the following exception,

W tensorflow/compiler/jit/mark_for_compilation_pass.cc:1412] (One-time warning): Not using XLA:CPU for cluster because envvar TF_XLA_FLAGS=--tf_xla_cpu_global_jit was not set.  If you want XLA:CPU, either set that envvar, or use experimental_jit_scope to enable XLA:CPU.  To confirm that XLA is active, pass --vmodule=xla_compilation_cache=1 (as a proper command-line flag, not via TF_XLA_FLAGS) or set the envvar XLA_FLAGS=--xla_hlo_profile.

I have set the environment variable as said in the error message. Feel free to close this PR if you think you have a found a solution. This is just an attempt.

Other comments

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

  • No release notes entry will be added for this pull request.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->
Fixes #17105 

#### Brief description of what is fixed or changed
I noticed that the master build is failing due to the following exception,
```
W tensorflow/compiler/jit/mark_for_compilation_pass.cc:1412] (One-time warning): Not using XLA:CPU for cluster because envvar TF_XLA_FLAGS=--tf_xla_cpu_global_jit was not set.  If you want XLA:CPU, either set that envvar, or use experimental_jit_scope to enable XLA:CPU.  To confirm that XLA is active, pass --vmodule=xla_compilation_cache=1 (as a proper command-line flag, not via TF_XLA_FLAGS) or set the envvar XLA_FLAGS=--xla_hlo_profile.
```
I have set the environment variable as said in the error message. Feel free to close this PR if you think you have a found a solution. This is just an attempt.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@czgdp1807 czgdp1807 closed this Jun 26, 2019

@czgdp1807 czgdp1807 deleted the czgdp1807:travis branch Jun 26, 2019

@czgdp1807 czgdp1807 referenced this pull request Jun 26, 2019

Merged

Fix tensorflow failing #17104

@czgdp1807 czgdp1807 restored the czgdp1807:travis branch Jun 26, 2019

@czgdp1807 czgdp1807 reopened this Jun 26, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 26, 2019

Codecov Report

Merging #17103 into master will increase coverage by 0.004%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           master    #17103       +/-   ##
============================================
+ Coverage    74.5%   74.504%   +0.004%     
============================================
  Files         623       623               
  Lines      161483    161339      -144     
  Branches    37896     37863       -33     
============================================
- Hits       120305    120205      -100     
+ Misses      35843     35813       -30     
+ Partials     5335      5321       -14
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

I believe that the approach being currently used is a bit repetitive. Is it possible to skip all the tests which depend on tensorflow in one go if it is not installed rather than checking in each test and skipping. may be a wrapper function can be made inside which other tensorflow dependent tests can be included.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

I think the tensorflow deprecation warnings are removed as can be seen in this job.

@czgdp1807 czgdp1807 changed the title Setting global env var Preparing sympy for tensorflow release Jun 27, 2019

@sylee957

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

I think that some deprecation warnings are still remaining.
Can you take a look at https://travis-ci.org/sympy/sympy/jobs/550787877 ?

W0626 13:55:20.459934 139826228270912 deprecation_wrapper.py:119] From /home/travis/miniconda/envs/test-environment/lib/python3.6/site-packages/sympy-1.5.dev0-py3.6.egg/sympy/printing/tests/test_tensorflow.py:28: The name tf.Session is deprecated. Please use tf.compat.v1.Session instead.
W0626 14:04:24.470630 139859698296640 deprecation.py:323] From <lambdifygenerated-143>:2: add_dispatch_support.<locals>.wrapper (from tensorflow.python.ops.array_ops) is deprecated and will be removed in a future version.
Instructions for updating:
@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

I think that some deprecation warnings are still remaining.

Thanks for notifying, I will correct those too.

@oscargus

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

Is it good to remove deprecation warnings? I see the point of not getting messed up test logs, but it will also reduce the probability that someone does something about it before it is actually removed. Maybe one should at least open an issue about it? (Or maybe there is one already?)

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2019

I see the point of not getting messed up test logs, but it will also reduce the probability that someone does something about it before it is actually removed.

https://github.com/tensorflow/tensorflow/tree/r2.0 - I am not in touch with tensorflow so not tracking the latest releases but the link suggests that version 2.0 is going to be released. So, I think changes made are necessary otherwise the master build will fail in future. Please correct me if I have interpreted wrongly.

Maybe one should at least open an issue about it? (Or maybe there is one already?)

Yes, I referenced the issue in the description of the PR.

@oscargus

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

My thought is that if compat is removed it will break completely. But maybe that isn't the case here. I'm just thinking that if something is deprecated it will eventually be removed, not just accessible in a special way (through compat). But it may be different for different deprecation warnings.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

@oscargus I investigated a bit about the deprecation and found out that there are many changes being made, may be for the standardisation.
Though I still don't know why, tf.Session -> tf.compat.v1.Session. has been done?

As far as I can figure out from this and this, they are going from Session -> Function approach, hence, tf.Session is deprecated. That's really great idea, well, now, as you said that, may be compat is removed completely. I don't think it will happen, because, if that is done then the current code depending on tensorflow, will completely break. I think they are changing the namespace of Session to tf.compat.v1 for some backward compatibility, so may be they will not remove it in the near future. We can wait until they do it.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

From, tensorflow/tensorflow#26844 (comment), tensorflow/tensorflow#26844 (comment), tensorflow/tensorflow#26844 (comment), I conclude that compat.v1 is not going to be removed at least for an year.

.travis.yml Outdated
@@ -11,6 +11,7 @@ env:
- TEST_DOCTESTS="true" FASTCACHE="false" TEST_SETUP="true"
global:
- secure: "YIEZal9EBTL+fg2YmoZoS8Bvt3eAVUOZjb38CtqpzR2CCSXWoUk35KG23m2NknlY1iKfYJyt7XWBszT/VKOQEbWQq7PIakV4vIByrWacgBxy1x3WC+rZoW7TX+JJiL+y942qIYbMoNMMB8xFpE5RDLSjSecMpFhJJXoafVTvju8="
- TF_XLA_FLAGS=--tf_xla_cpu_global_jit

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 28, 2019

Member

What is the purpose of this? Does the module work if this variable isn't set? Can it be set in Python? If this is required for tensorflow to work, we should document it. Otherwise the module will work on Travis, but won't work anywhere else unless someone knows to set this environment variable.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 28, 2019

Author Member

It raises a one time warning, if not set. I have given the detailed warning in the description of the PR. By the way, I believe that it would have been mentioned in tensorflow docs. Do we still need to document it? I just set it so that logs don't have unnecessary warnings. If it is not required, then please remove it.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jun 28, 2019

Member

I see. Do you know what test is producing that? I don't really understand what the warning is talking about.

At any rate you should add a comment here explaining that this is to silence a warning in the tensorflow tests. It may be better to move it to the test_travis script, right before the optional dependency tests are called.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 28, 2019

Author Member

In fact, I also followed the instructions given in the warning. Actually, I am away from my system right now. I will update the PR soon. As this is not critical, so I will also look up into the details of the warning, it seems interesting.

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 2, 2019

Author Member

I read about the XLA at https://www.tensorflow.org/xla/overview. AFAICT, XLA is Accelerated Linear Algebra. The point is that XLA is in active development, so I believe that we should not use XLA for now. In fact, the warning is not that serious, so we can ignore that during the build.
Till now, I haven't figured out, how to silence that via code. By default, tensoflow is not compiled via XLA.
I will remove TF_XLA_FLAGS=--tf_xla_cpu_global_jit if no objection raised for 24 hours.

This comment has been minimized.

Copy link
@asmeurer

asmeurer Jul 2, 2019

Member

That seems fine. There are also some other warnings printed during the tests from external dependencies that are not so easy to get rid of. #14747

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jul 6, 2019

Author Member

I opened tensorflow/tensorflow#30308 for getting to know any trick to silence the warning but received no response. So, I am finally removing the TF_XLA_FLAGS=--tf_xla_cpu_global_jit.
Probably after that this PR will be done from my side and for adapting to new API of TF, I have opened #17149 and will draft a PR soon for fixing it. That PR can be left open until the installation of TF 2.0.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

So we really need to update the code to work properly with the new tensorflow API. I think this shouldn't be marked as fixing the issue just yet. We also shouldn't mark it as fixed until the CI actually installs tensorflow 2.0.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

Actually, deprecation warnings are being shown, so I thought to be early and be prepared. Well, it can be kept open until the release and installation of tf 2.0.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Don't get me wrong. We should definitely merge this in the interim to get rid of the warnings in the tests. But we need an open issue for proper tensorflow 2.0 support. Perhaps a separate issue would be better since they are separate things.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

@asmeurer I think you are right. I will raise an issue for it and may be I will work on it. As the API from TF 1.x to TF 2.x, is changed drastically so I think we should have to make change from Sessions approach to Functions. So, some investigation will be needed. I will draft a separate PR for incorporating the new API if possible.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

But we need an open issue for proper tensorflow 2.0 support.

Right, I will open the issue, if not done till now by anyone. This PR just aims to silence the warnings using compat.v1 API. The new PR will focus on adapting to the new API of tensorflow, without using compat.v1 and using the new approach of tensorflow.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

@sylee957 any more changes needed on it, to silence the warnings. refer #17103 (comment) too.

@sylee957

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Does this work before tensorflow 1.13 release?

>>> tf.__version__
'1.12.0'
>>> tf.compat.v1
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'tensorflow._api.v1.compat' has no attribute 'v1'

And 1.13 may be fairly recent, and they may still update some old versions.

@czgdp1807

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2019

I think tensorflow 1.14 is already released and AFAIK, they will not go back to previous design(and hence versions), they were using, so, tf.compat.v1. But still if you think that we should wait for TF 2.0 to be installed then no issues, let's keep this open.
tf.compat.v1 will be removed in TF 3.0, which is very far in future.
For clarification, does CI always installs the latest versions of tensorflow?

@sylee957

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I think that CI simply uses conda commands to install the latest package. So it can install the latest version by default, or the version can be pinned down to TEST_OPT_DEPENDENCY environment variable.

However, the thing I'm afraid of is that the tensorflow.compat module seems like being incremented only after the new version is released. So, the tests should be skipped if the tensorflow isn't the latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.