Bug 219609 - [NEW PORT] science/py-tensorflow: Computation using data flow graphs for scalable machine learning
Summary: [NEW PORT] science/py-tensorflow: Computation using data flow graphs for scal...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Li-Wen Hsu
URL: https://reviews.freebsd.org/D11194
Keywords: feature, needs-qa
Depends on: 219840 220206
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-28 08:06 UTC by Jov
Modified: 2017-08-16 01:27 UTC (History)
8 users (show)

See Also:
amutu: maintainer-feedback+


Attachments
tensorflow.shar (297.37 KB, text/plain)
2017-05-28 08:06 UTC, Jov
no flags Details
tensorflow.shar v2 (297.85 KB, text/plain)
2017-05-28 15:55 UTC, Jov
no flags Details
tensorflow.shar v3 (297.86 KB, text/plain)
2017-05-29 06:03 UTC, Jov
no flags Details
tensorflow_v4.shar (297.93 KB, text/plain)
2017-06-09 01:25 UTC, Jov
no flags Details
tensorflow_v5.shar (297.99 KB, text/plain)
2017-06-09 11:04 UTC, Jov
amutu: maintainer-approval+
Details
tensorflow_v6.shar (298.01 KB, text/plain)
2017-06-10 03:58 UTC, Jov
no flags Details
tensorflow_v7.shar (462.75 KB, text/plain)
2017-06-20 03:16 UTC, Jov
amutu: maintainer-approval+
Details
tensorflow_v8.shar (463.22 KB, text/plain)
2017-06-22 09:51 UTC, Jov
no flags Details
tensorflow_v9.shar (463.10 KB, text/plain)
2017-06-25 10:21 UTC, Jov
no flags Details
tensorflow_v10.shar (320.52 KB, text/plain)
2017-06-27 11:53 UTC, Jov
no flags Details
tensorflow_v11.shar (320.42 KB, text/plain)
2017-06-29 06:27 UTC, Jov
no flags Details
tensorflow.diff (462.23 KB, patch)
2017-07-12 09:00 UTC, Jov
no flags Details | Diff
tensorflow.diff (462.23 KB, patch)
2017-07-13 11:59 UTC, Jov
no flags Details | Diff
py-tensorflow.shar (463.26 KB, text/plain)
2017-07-19 10:51 UTC, Jov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jov 2017-05-28 08:06:27 UTC
Created attachment 182983 [details]
tensorflow.shar

dirty port of tensorflow:

TensorFlow is an open source software library for numerical computation using
data flow graphs. Nodes in the graph represent mathematical operations, while
the graph edges represent the multidimensional data arrays (tensors)
communicated between them. The flexible architecture allows you to deploy
computation to one or more CPUs or GPUs in a desktop, server, or mobile device
with a single API. TensorFlow was originally developed by researchers and
engineers working on the Google Brain Team within Google's Machine Intelligence
research organization for the purposes of conducting machine learning and deep
neural networks research, but the system is general enough to be applicable in
a wide variety of other domains as well.

WWW: https://www.tensorflow.org
Comment 1 Kubilay Kocak freebsd_committer 2017-05-28 09:40:20 UTC
Thank you for your submission Jov. 

Can you please confirm that this port passes QA (portlint and poudriere in particular).
Comment 2 Jov 2017-05-28 15:00:35 UTC
(In reply to Kubilay Kocak from comment #1)
portlint pass and has 2 warining,I will submit v2 patch which has 1 warning:
WARN: Makefile: possible use of absolute pathname "/proc)".
caused by:
mount -t procfs proc /proc
I don't know how to get rid of this,but I think it's harmless?

I do not run poudriere for this port because the mount /proc and also bazel download should do some configuration to poudriere.I create this port on 11.0-RELEASE and test the build on a fresh installed 12-current vm(installed using FreeBSD-12.0-CURRENT-amd64-20170510-r318137-disc1.iso).So for 11.0/12 it works.

this port do the build mainly as this post:
https://forums.freebsd.org/threads/59479/

v2 also restrict the build only on 11.0+.
Comment 3 Jov 2017-05-28 15:55:19 UTC
Created attachment 183004 [details]
tensorflow.shar v2

only build on 11+
add run time depends using pip package meta info
Comment 4 Jov 2017-05-29 06:03:14 UTC
Created attachment 183026 [details]
tensorflow.shar v3

v3 patch

1.mv output of bazel to WRKDIR. the default output of bazel is /root/.cache which out of WRKDIR,may left garbage even after make clean.
2.patch the src after confiure,then build.v1 and v2 patch do the sequence of  build-failed-patch-build,very dirty.
Comment 5 Kubilay Kocak freebsd_committer 2017-05-30 04:32:03 UTC
Initial review:

1) USES=python implies {RUN,BUILD}_DEPENDS on Python (can remove python:lang/python). Additionally, use of python:lang/python is always incorrect, as it only means 'depends on the `python` symlink' not 'depends on python'. Only use USES=python:<args>

2) Upstream stipulates Python 2.7 *and* 3.3+ support. USES=python:2.7 limits it to 2.7 only.

3) Regarding BROKEN (${OSVERSION} < 1100101):

3a) clang and llvm 3.8 exist in ports and can be depended on. If there is nothing intrinsic or super-complex preventing other versions of FreeBSD from building tensorflow, it should do so.

3b) If clang is not a requirement for building tensorflow, it should not be a specific dependency. I see no mention of it on the 'building tensorflow from sources' document, but further, there *is* mention of builds with GCC, which motivates this comment.

USES=compiler:<args> should be used if a specific feature is required (See Porters Handbook -- USES -- compiler for details [1])

4) Vendored dependencies should be extracted/removed and those ports/packages in the tree depended on instead.

5) It's unlikely that all python dependencies are both RUN and BUILD dependencies. Please make these as accurate to their actual dependency types as possible.

6) -march=native should not be used for CC/CFLAGS. Binary packages are built from ports and build host CPU architecture has no bearing on end-user hardware, which may be different.

7) configuration (in this case ./configure) steps should be run in the do-configure target, not the build target. Additionally and ideally, using CONFIGURE_TARGET and other CONFIGURE_* variables, without requiring a custom do-configure target override. If bazel is a pre build or 'configuration' step, it should be done in the configure target as well.

8) Patching of WRKSRC files should occur in {pre,post}-patch target, not build target.

9) Was using the port/package of pip tested rather than configuring/building the  bundled tensorflow/tools/pip_package? The former should be used if and where possible.

This port requires additional QA iterations

[1] https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/book.html#uses-compiler
Comment 6 Jov 2017-05-30 16:01:26 UTC
(In reply to Kubilay Kocak from comment #5)
Thanks for your review,from your comment I learn more about freebsd ports,I appreciate it! My reply is inlined:

>Initial review:

>1) USES=python implies {RUN,BUILD}_DEPENDS on Python (can remove python:lang/python). Additionally, use of python:lang/python is always incorrect, as it only means 'depends on the `python` symlink' not 'depends on python'. Only use USES=python:<args>
will fix

>2) Upstream stipulates Python 2.7 *and* 3.3+ support. USES=python:2.7 limits it to 2.7 only.
will fix by using USES=python:2.7+,and test python3.

>3) Regarding BROKEN (${OSVERSION} < 1100101):

>3a) clang and llvm 3.8 exist in ports and can be depended on. If there is nothing intrinsic or super-complex preventing other versions of FreeBSD from building tensorflow, it should do so.
see blow 3b

>3b) If clang is not a requirement for building tensorflow, it should not be a specific dependency. I see no mention of it on the 'building tensorflow from sources' document, but further, there *is* mention of builds with GCC, which motivates this comment.
I have try to use gcc but it is failed for bazel to find gcc.If I set --compiler=gcc-5.4.0,bazel report error:
(cd /usr/ports/science/tensorflow/work/tensorflow-1.1.0 && bazel --output_user_root=/usr/ports/science/tensorflow/work/bazel_ot build --compiler=gcc-5.4.0  --config=opt //tensorflow/tools/pip_package:build_pip_package --verbose_failures)
ERROR: No toolchain found for --cpu='freebsd' --compiler='gcc-5.4.0'. Valid toolchains are: [
  --cpu='armeabi-v7a' --compiler='compiler' --glibc='armeabi-v7a',
  --cpu='local' --compiler='compiler' --glibc='local',
  --cpu='darwin' --compiler='compiler' --glibc='macosx',
  --cpu='freebsd' --compiler='compiler' --glibc='local',
  --cpu='x64_windows' --compiler='windows_mingw' --glibc='local',
  --cpu='x64_windows' --compiler='windows_msys64_mingw64' --glibc='local',
  --cpu='x64_windows' --compiler='windows_clang' --glibc='local',
  --cpu='x64_windows' --compiler='windows_msys64' --glibc='local',
  --cpu='x64_windows_msvc' --compiler='cl' --glibc='msvcrt140',
  --cpu='ios_x86_64' --compiler='compiler' --glibc='ios',

After read this,I think I need to write a toolchain config to work with gcc: 
https://github.com/bazelbuild/bazel/wiki/Building-with-a-custom-toolchain

>USES=compiler:<args> should be used if a specific feature is required (See Porters Handbook -- USES -- compiler for details [1])
will fix after I solve 3b.

>4) Vendored dependencies should be extracted/removed and those ports/packages in the tree depended on instead.
tensorflow use bazel manage dependencises.
the tensorflow-1.1.0/tensorflow/workspace.bzl contains dependencises info like this:
native.new_http_archive(
      name = "org_pocoo_werkzeug",
      urls = [
          "http://bazel-mirror.storage.googleapis.com/pypi.python.org/packages/b7/7f/44d3cfe5a12ba002b253f6985a4477edfa66da53787a2a838a40f6415263/Werkzeug-0.11.10.tar.gz",
          "https://pypi.python.org/packages/b7/7f/44d3cfe5a12ba002b253f6985a4477edfa66da53787a2a838a40f6415263/Werkzeug-0.11.10.tar.gz",
      ],
      strip_prefix = "Werkzeug-0.11.10",
      sha256 = "cc64dafbacc716cdd42503cf6c44cb5a35576443d82f29f6829e5c49264aeeee",
      build_file = str(Label("//third_party:werkzeug.BUILD")),
  )

And the count of dependencies is bigger than normal ports: 
grep -c 'sha256 =' workspace.bzl 
90
some dependencises even are not available on FreeBSD ports,like grpc.
It is a lot work to make this fixed and need to patch the bazel config ,this will make future update difficult also.

>5) It's unlikely that all python dependencies are both RUN and BUILD dependencies. Please make these as accurate to their actual dependency types as possible.
will fixed

>6) -march=native should not be used for CC/CFLAGS. Binary packages are built from ports and build host CPU architecture has no bearing on end-user hardware, which may be different.
will fixed by using $CFLAGS

>7) configuration (in this case ./configure) steps should be run in the do-configure target, not the build target. Additionally and ideally, using CONFIGURE_TARGET and other CONFIGURE_* variables, without requiring a custom do-configure target override. If bazel is a pre build or 'configuration' step, it should be done in the configure target as well.
I hope to do this as you say,but the configure of this port is not gnu autoconfig or similar,it is a bash script! It used to read user input from stdin and set some variables then do bazel fetch.
I will fix this by moving configure to do-configure target.

>8) Patching of WRKSRC files should occur in {pre,post}-patch target, not build target.
As 4 and 7 mentioned, the configure will fetch all dependencises and the patch is for the grpc and protobuf which only are available after configure.
So I will fix tis by moving patch to post-configure target.

>9) Was using the port/package of pip tested rather than configuring/building the  bundled tensorflow/tools/pip_package? The former should be used if and where possible.
yes I understand.but there is no pip package for freebsd,tensorflow offical suport 
 is linux and macos.tensorflow/tools/pip_package will build one,it will include all the dependencises python lib and C/C++ libs to the pip package,you can see the pkg-plist.

>This port requires additional QA iterations
I will setup poudriere to test this when all of the above fixed.
Comment 7 Mathieu Arnold freebsd_committer 2017-06-07 13:15:56 UTC
Quick review, the mount /proc has to go.
Ports are built as a regular user, there is no way it can possibly ever do anything.
Comment 8 Jov 2017-06-09 01:25:27 UTC
Created attachment 183346 [details]
tensorflow_v4.shar

v4 patch:
1.fix build for freebsd 10.x
2.add python prefix to pkg name
3.clean up build depends and run depends
4.move configure phase to do-configure
5.respect CFLAGS
6.fix build for i386
7.use bazel batch mode so not left java process after build

QA:
portlint:no error,1 warning for mount /proc
poudriere test port tested and pass:
11.0R-amd64-py36
11.0R-amd64-py27
11.0R-i386-py27
10.3R-amd64-py27
Comment 9 Jov 2017-06-09 01:33:31 UTC
(In reply to Kubilay Kocak from comment #5)
Hi Kubilay,
I try to remove the python:lang/python from BUILD_DEPENDS as you say, the build will fail to find /usr/local/bin/python for testport.And I find python27 do not depend on lang/python so I think the USES= python:2.7+ do not install lang/python.So I do not remove the python depends.
Comment 10 Kubilay Kocak freebsd_committer 2017-06-09 05:08:42 UTC
(In reply to Jov from comment #9)

Software packages should be PEP-394 [1] compatible in terms of how they invoke Python.

Additionally, for Python package/software ports in FreeBSD, they should invoke a specific Python version (pythonX.Y, see variable PYTHON_CMD) and be as PEP-394 compliant as possible.

Both of the above apply at build-time and run-time. Where `python` points (or if it is present at all) can change at any time at the discretion of the user.

That aside, if and when present, can only happen to point to a 'default' version of Python, which has no necessary correlation with what Python environment version the software has been installed in. 

For files/scripts, maintainers may use USES=shebangfix [2] and SHEBANG_FILES to correct shebang lines 

[1] https://www.python.org/dev/peps/pep-0394/
[2] https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/book.html#uses-shebangfix
Comment 11 Jov 2017-06-09 11:01:52 UTC
(In reply to Kubilay Kocak from comment #10)
Thank you for your comment.I fixed the problem and will submit v5 patch.

By the way, is there any wiki pages or articles about the python port FAQ?
Comment 12 Jov 2017-06-09 11:04:09 UTC
Created attachment 183358 [details]
tensorflow_v5.shar

v5 patch:

1.remove lang/python depend
2.add python shebangfix

test pass poudriere 11R-amd64
Comment 13 Mathieu Arnold freebsd_committer 2017-06-09 11:50:56 UTC
The python_CMD definition is not needed, remove it.

post-patch and post-configure: Never call SED directly, we have REINPLACE_CMD for that.

The procfs mount must be removed. Packages are build as regular user, no regular user can ever mount anything. Also, they are built in jails where mounting is not allowed, so even if root was building, it could not mount either.

Please merge do-configure and post-configure, there is no reason to have two targets when only one is needed.

In do-configure, you seem to be setting enviroment variables without using SETENV, please add it.

do-build, same, use SETENV.

The do-install target is a bit strange, it looks like you are "installing" files by moving them with MV. you should instead be copying them with COPYTREE_SHARE.
Comment 14 Jov 2017-06-10 03:54:53 UTC
(In reply to Mathieu Arnold from comment #13)
Thanks Mathieu!all fixed by v6 patch
Comment 15 Jov 2017-06-10 03:58:41 UTC
Created attachment 183372 [details]
tensorflow_v6.shar

1.remove python_CMD
2.use REINPLACE_CMD instead of SED
3.remove mount /proc
4.merge do-configure and post-configure
5.use SETENV to set environment variables
6.use COPYTREE_SHARE instead of MV
Comment 16 Jov 2017-06-10 03:59:26 UTC
(In reply to Jov from comment #15)
Also poudriere testport pass
Comment 17 Mathieu Arnold freebsd_committer 2017-06-12 06:46:39 UTC
You should not overwrite do-patch but use post-patch for the REINPLACE_CMD.
Comment 18 Jov 2017-06-14 04:02:33 UTC
NEW patch move to here:
https://reviews.freebsd.org/D11194
Comment 19 Jov 2017-06-20 03:16:54 UTC
Created attachment 183637 [details]
tensorflow_v7.shar

v7:
1.fix network access at do-configure
2.update port version to 1.2.0, which release 2 days ago.
Comment 20 Jov 2017-06-22 09:51:53 UTC
Created attachment 183701 [details]
tensorflow_v8.shar

v8:
fix tensorflow-1.2.0 runtime depends, these dependencies com from the pip package meta data.

Upstream 1.2.0 add a new runtime depend named backports.weakref, this software do not port to FreeBSD, so I port it. PR:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=220206
Comment 21 Jov 2017-06-25 10:21:00 UTC
Created attachment 183782 [details]
tensorflow_v9.shar

As I make the bazel_clang38 port do not use /porc mount,see:https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219840#c4, now tensorflow depend bazel_clang38 on all version so we can build on all version without monting /proc.
Comment 22 Jov 2017-06-27 11:53:10 UTC
Created attachment 183836 [details]
tensorflow_v10.shar

V10:
per lwshu@, use PATCHFILES to download large patch files.
Comment 23 Jov 2017-06-29 06:27:23 UTC
Created attachment 183901 [details]
tensorflow_v11.shar

sync with https://reviews.freebsd.org/D11194:

1.add version to external patch files per lwhsu@
2.remove unnecessary SHEBANG_LANG and merge sed cmd per mat@
Comment 24 Jov 2017-07-12 09:00:47 UTC
Created attachment 184298 [details]
tensorflow.diff

sync patch with https://reviews.freebsd.org/D11194
Comment 25 Jov 2017-07-13 11:59:26 UTC
Created attachment 184326 [details]
tensorflow.diff

Change the build time depend bazel_clang38 to bazel-clang38.
Comment 26 Yuri Victorovich freebsd_committer 2017-07-16 00:49:06 UTC
Could you please update to the current version 1.2.1?
Comment 27 Jov 2017-07-16 03:08:09 UTC
(In reply to Yuri Victorovich from comment #26)
I will update the version after the blocker solved: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219840

If you really want to use 1.2.1,you can try to change the PORTVERSION to 1.2.1 and make makesum, then make install. The offline third party should work as someone tested it on Linux.
Comment 28 Yuri Victorovich freebsd_committer 2017-07-16 17:16:42 UTC
IMO, 219840 shouldn't be a blocker for tensorflow.

You should add :
> .if ${OSVERSION} >= 1100000
> BUILD_DEPENDS+=bazel:devel/bazel
> USE_PROCFS=yes
> .else
> BROKEN "Waiting for devel/bazel-clang38, see bug#219840"
> .endif

No need to block it for everybody for non-essential reasons.
Comment 29 Yuri Victorovich freebsd_committer 2017-07-16 17:27:41 UTC
Also, did you consider naming it science/py-tensorflow?
Everything it installs is under site-packages/.
Comment 30 Jov 2017-07-17 02:05:17 UTC
(In reply to Yuri Victorovich from comment #28)
For broken, please see comment 5.
For procfs, please see comment 7.
Comment 31 Jov 2017-07-17 02:06:20 UTC
(In reply to Yuri Victorovich from comment #29)
The package name already contains pyXX-.
Comment 32 Yuri Victorovich freebsd_committer 2017-07-19 06:34:54 UTC
(In reply to Jov from comment #31)

The directory should be science/py-tensorflow

Look how project depending on tensor lists it in setup.py: https://github.com/tensorflow/magenta/blob/master/magenta/tools/pip/setup.py#L38
It is listed among other python packages. Also all files it installs are under site-packages/.
Comment 33 Jov 2017-07-19 10:51:19 UTC
Created attachment 184500 [details]
py-tensorflow.shar

1.update to tensorflow-1.2.1.
2.change dirname to py-tensorflow.
3.add freebsd version condition to make 11.x+ use devel/bazel install bazel-clang38 because procfs patch already included.
4.add PLIST_SUB to make plist file easy maintain when update.
Comment 34 Jov 2017-08-08 07:22:07 UTC
Hi Kubilay, I see you added 219794 as a dependent, but I think PR 219794 do nothing with Tensorflow. Tensorflow builds and works well when PR 219794 unresolved. It should only block PRs about py-backports*. Is it possible to remove the dependency relations, and let this PR move on?
Comment 35 Yuri Victorovich freebsd_committer 2017-08-08 07:27:56 UTC
(In reply to Jov from comment #34)

+1 I agree. IMO, delete the dependency and commit.
Comment 36 Kubilay Kocak freebsd_committer 2017-08-08 10:05:51 UTC
(In reply to Jov from comment #34)

I can't explain why I added it (possible typo)

If this port doesn't provide or touch backports directories or files, it doesn't depend, otherwise it does.
Comment 37 Kubilay Kocak freebsd_committer 2017-08-08 10:13:40 UTC
I see why:

tensorflow depends on backports.weakref which led to bug 220749 which was closed duplicate of bug 219794

Since backports.weakref is already in the tree (with its conflicts) resolving bug 219794 is not an 'actual' blocker, *as long* as installing tensorflow does not install two dependencies that each depend on different and conflicting backports ports. I believe this would have come up in testing if it did.
Comment 38 commit-hook freebsd_committer 2017-08-15 12:08:30 UTC
A commit references this bug:

Author: lwhsu
Date: Tue Aug 15 12:08:03 UTC 2017
New revision: 447979
URL: https://svnweb.freebsd.org/changeset/ports/447979

Log:
  Ad TensorFlow 1.2.1

  PR:		219609
  Submitted by:	Jov <amutu@amutu.com>

Changes:
  head/science/Makefile
  head/science/py-tensorflow/
  head/science/py-tensorflow/Makefile
  head/science/py-tensorflow/distinfo
  head/science/py-tensorflow/files/
  head/science/py-tensorflow/files/patch-WORKSPACE
  head/science/py-tensorflow/files/patch-tensorflow__third__party_grpc_include_grpc_impl_codegen_port__platform.h
  head/science/py-tensorflow/files/patch-tensorflow__third__party_protobuf_src_google_protobuf_compiler_plugin.pb.cc
  head/science/py-tensorflow/files/patch-tensorflow__third__party_protobuf_src_google_protobuf_compiler_plugin.pb.h
  head/science/py-tensorflow/files/patch-tensorflow_workspace.bzl
  head/science/py-tensorflow/pkg-descr
  head/science/py-tensorflow/pkg-plist
Comment 39 Li-Wen Hsu freebsd_committer 2017-08-15 12:10:16 UTC
With minor portlint(1) changes.
Comment 40 Jov 2017-08-16 01:27:23 UTC
Great work! Thanks, Li Wen!