Make init.py files creation optional #4470

mouadino · 2018-01-17T11:22:56Z

Introduce a new attribute to py_binary and py_test to control whether to
create __init__.py or not.

TODO:

Sign CLA (waiting for my company input on how to proceed).
Test to see if my original reported issue is fixed with this change, especially how this works with https://github.com/bazelbuild/rules_python#importing-pip-dependencies.
Add automated tests, although I am not sure how to do it, any advice is much appreciated.
Understand why flipping the new flag doesn't create/delete __init__.py files, b/c currently running clean is required after flipping the flag.

bazel-io · 2018-01-17T11:22:58Z

Can one of the admins verify this patch?

googlebot · 2018-01-17T11:22:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

mouadino · 2018-01-17T11:35:20Z

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java

+          Controls whether to create __init__.py files in paths where they don't exists
+          or not, default is to true for backward compatibility and to support legacy behavior.
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(attr("legacy_create_init", BOOLEAN).value(true))


Please advise on the name of the attribute, I went with the legacy_ convention since I saw it used for some attributes but I am not sure if it's the right use case.

I was wondering if this should be an enum or integer, in case we want more than two behaviors. But I can't think of any third thing we might do.

Can we call it legacy if we don't have a specific plan to switch everything to the new behavior? I'm ok with it, but if you wanted a different name, maybe implict_create_init_py?

You could expand the comment a bit to explain the legacy behavior:

Whether to implicitly create empty __init__.py files in the runfiles tree. These are created in every directory containing Python source code or shared libraries, and every parent directory of those directories. Default is true for backward compatibility. If false, the user is responsible for creating __init__.py files (empty or not) and adding them to `srcs` or `deps` of Python targets as required.

I like your description will adapt, concerning the attribute name IMHO we should drop the behavior in the future and that's how I understood it here bazelbuild/rules_python#55 (comment).

mouadino · 2018-01-18T10:11:50Z

I signed it!

googlebot · 2018-01-18T10:11:52Z

CLAs look good, thanks!

ulfjack · 2018-01-26T08:54:58Z

Thanks for the patch! Code changes look good. Unfortunately, we haven't open sourced some of the py_* tests yet :-(, so it's not easy to write unit tests.

@lberki, what do you think?

lberki · 2018-01-26T10:04:28Z

/cc @scentini

(sorry for the late reply, this surfaced in my inbox after @ulfjack 's reply)

@ulfjack : do you think this is desirable functionality? There is clearly need for it, but our Python rules aren't very well shepherded at the moment and it's One More Knob. I'd say "anyways, here's Wonderwall^H the new attribute", but would like to have a second opinion.

ulfjack · 2018-01-26T10:47:49Z

See bazelbuild/rules_python#55 for context. This seems to be an issue for a number of users, and @duggelz said that the current behavior is legacy.

lberki · 2018-01-26T12:54:32Z

Let me just write an integration test which you can then extend....

ulfjack · 2018-01-26T13:09:28Z

How about open sourcing PyBinaryConfiguredTarget instead? Writing more integration tests will just make our presubmits run even more slowly. :-P

lberki · 2018-01-26T13:11:45Z

I looked at it and PyBinaryConfiguredTarget is mostly about testing Python/C++ interaction, which we don't have in the OSS version. Also, I'd much rather we have a test that actually tests how the runfiles tree looks like instead of checking some data structures that should result in a runfiles tree of a given shape.

I'm all for fast tests, but at the same time, we should have a few integration tests just to be sure.

lberki · 2018-01-26T13:20:01Z

FYI: Code review for the test sent out: https://bazel-review.googlesource.com/c/bazel/+/34750

duggelz

This is terrific.

duggelz · 2018-01-26T19:04:04Z

src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java

+          Controls whether to create __init__.py files in paths where they don't exists
+          or not, default is to true for backward compatibility and to support legacy behavior.
+          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+          .add(attr("legacy_create_init", BOOLEAN).value(true))


I was wondering if this should be an enum or integer, in case we want more than two behaviors. But I can't think of any third thing we might do.

Can we call it legacy if we don't have a specific plan to switch everything to the new behavior? I'm ok with it, but if you wanted a different name, maybe implict_create_init_py?

You could expand the comment a bit to explain the legacy behavior:

Whether to implicitly create empty __init__.py files in the runfiles tree. These are created in every directory containing Python source code or shared libraries, and every parent directory of those directories. Default is true for backward compatibility. If false, the user is responsible for creating __init__.py files (empty or not) and adding them to `srcs` or `deps` of Python targets as required.

duggelz · 2018-01-26T19:16:14Z

src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java

+
+    if (ruleContext.attributes().get("legacy_create_init", Type.BOOLEAN)) {
+      builder.setEmptyFilesSupplier(PythonUtils.GET_INIT_PY_FILES);
+    }


I'm a little dumbfounded that it's this simple. I guess that means the Bazel code is well structured.

Indeed it is, if I can do it then everyone can :)

lberki · 2018-01-30T18:08:33Z

Example integration test submitted: bb9f19d

@mouadino , can you add a test for this change in the spirit of said change?

mouadino · 2018-01-31T12:01:46Z

@lberki Awesome, will try to adapt.

mouadino · 2018-02-06T19:06:32Z

Just a small update, I didn't forget about this, it have been some busy two weeks with conferences and sickness, I will try to update it in the weekend, apologies for any issues that the delay may have caused.

lberki · 2018-02-07T12:34:31Z

No problem, looking forward to it!

Introduce a new attribute to py_binary and py_test to control whether to create `__init__.py` or not. Fixes bazelbuild/rules_python#55

Also address review comment concerning documentation.

mouadino · 2018-02-10T19:22:30Z

Automated tests added @lberki please have a look.

lberki · 2018-02-14T08:32:51Z

Erm, sorry for the late reply. Looks good, I'll merge this. Thanks for the patience!

lberki · 2018-02-14T08:38:51Z

/cc @scentini

Wait a moment... is it still the case that flipping the flag doesn't delete/add the __init__.py files? I'd be surprised if that was the case and it's not for you to fix in any case, but that sounds somewhat scary.

mouadino · 2018-02-14T08:57:52Z

Yes its still the case, the way I understood it, and please correct me if I am wrong, is that Bazel in general doesn’t track files that it doesn’t know off (this issue is common in tools that works with files like configuration management), so disabling a behaviour that was creating some files will not make this files disappear afterwards only a clean run will do that. One way of fixing this is instead of telling Bazel to not create __init__.py files, Bazel should insure that no __init__.py is there that doesn’t have an equivalent in workspace, in practical sense what we can do is to start by deleting all __init__.py files as pre step, thoughts?

…

On Wed 14. Feb 2018 at 09:39, lberki ***@***.***> wrote: /cc @scentini <https://github.com/scentini> Wait a moment... is it still the case that flipping the flag doesn't delete/add the *init*.py files? I'd be surprised if that was the case and it's not for you to fix in any case, but that sounds somewhat scary. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4470 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzBc_7lRF3D92Jmau0bZLgwazM2ynn2ks5tUpungaJpZM4RhMIs> .

lberki · 2018-02-14T08:59:46Z

The __init.py__ files in question are in the runfiles trees, which Bazel does know about. If those are not updated, that's a grave correctnes bug that should be looked into.

lberki · 2018-02-14T13:11:44Z

Will be rolled back then resubmitted; this change breaks some tests.

*** Reason for rollback *** Breaks Kokoro and I accidentally submitted the change without presubmit checks. *** Original change description *** Make __init__.py files creation optional Introduce a new attribute to py_binary and py_test to control whether to create `__init__.py` or not. Fixes bazelbuild/rules_python#55 Closes #4470. PiperOrigin-RevId: 185676592

*** Reason for rollback *** Remove example changes; those need to build with the last Bazel release. *** Original change description *** Automated rollback of commit 0f9c6ea. *** Reason for rollback *** Breaks Kokoro and I accidentally submitted the change without presubmit checks. *** Original change description *** Make __init__.py files creation optional Introduce a new attribute to py_binary and py_test to control whether to create `__init__.py` or not. Fixes bazelbuild/rules_python#55 Closes #4470. PiperOrigin-RevId: 185806241

mouadino · 2018-02-16T06:35:48Z

I am a bit lost, @lberki can you explain what's the status of the fix?

lberki · 2018-02-16T09:03:17Z

Erm, this is indeed confusing -- I merged your change, then had to roll it back, for it broke some tests and I accidentally submtited it without checking the tests, then rolled back the rollback after fixing the tests, so it should be now on HEAD. Its final version is e9c885a .

brandjon · 2019-03-14T20:20:34Z

FTR I just tried to reproduce the incremental correctness issue where __init__.py is not being deleted after flipping the attribute value and rebuilding. It worked fine for me.

googlebot added the cla: no label Jan 17, 2018

mouadino force-pushed the py-init-files-optional branch 2 times, most recently from 48cfa4b to ea3e8f7 Compare January 17, 2018 11:25

mouadino commented Jan 17, 2018

View reviewed changes

googlebot added cla: yes and removed cla: no labels Jan 18, 2018

mouadino mentioned this pull request Jan 19, 2018

Bazel breaks python import with __init__.py bazelbuild/rules_python#55

Closed

iirina requested a review from lberki January 22, 2018 16:28

iirina assigned lberki Jan 22, 2018

duggelz reviewed Jan 26, 2018

View reviewed changes

nfelt mentioned this pull request Feb 1, 2018

'tensorboard.plugins.projector' has no attribute 'ProjectorConfig' tensorflow/tensorboard#926

Closed

mouadino and others added 2 commits February 10, 2018 19:09

Make __init__.py files creation optional

232bbd0

Introduce a new attribute to py_binary and py_test to control whether to create `__init__.py` or not. Fixes bazelbuild/rules_python#55

Add integration tests for new flag

1211395

Also address review comment concerning documentation.

mouadino force-pushed the py-init-files-optional branch from ea3e8f7 to 1211395 Compare February 10, 2018 19:21

bazel-io closed this in 0f9c6ea Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make init.py files creation optional #4470

Make init.py files creation optional #4470

mouadino commented Jan 17, 2018 •

edited

Loading

bazel-io commented Jan 17, 2018

googlebot commented Jan 17, 2018

mouadino Jan 17, 2018 •

edited

Loading

duggelz Jan 26, 2018

mouadino Jan 28, 2018

mouadino commented Jan 18, 2018

googlebot commented Jan 18, 2018

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018 •

edited

Loading

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018

lberki commented Jan 26, 2018

duggelz left a comment

duggelz Jan 26, 2018

duggelz Jan 26, 2018

mouadino Jan 28, 2018

lberki commented Jan 30, 2018 •

edited

Loading

mouadino commented Jan 31, 2018

mouadino commented Feb 6, 2018

lberki commented Feb 7, 2018

mouadino commented Feb 10, 2018

lberki commented Feb 14, 2018 •

edited

Loading

lberki commented Feb 14, 2018 •

edited

Loading

mouadino commented Feb 14, 2018 via email

lberki commented Feb 14, 2018

lberki commented Feb 14, 2018

mouadino commented Feb 16, 2018

lberki commented Feb 16, 2018 •

edited

Loading

brandjon commented Mar 14, 2019

Make __init__.py files creation optional #4470

Make __init__.py files creation optional #4470

Conversation

mouadino commented Jan 17, 2018 • edited Loading

bazel-io commented Jan 17, 2018

googlebot commented Jan 17, 2018

mouadino Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

duggelz Jan 26, 2018

Choose a reason for hiding this comment

mouadino Jan 28, 2018

Choose a reason for hiding this comment

mouadino commented Jan 18, 2018

googlebot commented Jan 18, 2018

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018 • edited Loading

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018

ulfjack commented Jan 26, 2018

lberki commented Jan 26, 2018

lberki commented Jan 26, 2018

duggelz left a comment

Choose a reason for hiding this comment

duggelz Jan 26, 2018

Choose a reason for hiding this comment

duggelz Jan 26, 2018

Choose a reason for hiding this comment

mouadino Jan 28, 2018

Choose a reason for hiding this comment

lberki commented Jan 30, 2018 • edited Loading

mouadino commented Jan 31, 2018

mouadino commented Feb 6, 2018

lberki commented Feb 7, 2018

mouadino commented Feb 10, 2018

lberki commented Feb 14, 2018 • edited Loading

lberki commented Feb 14, 2018 • edited Loading

mouadino commented Feb 14, 2018 via email

lberki commented Feb 14, 2018

lberki commented Feb 14, 2018

mouadino commented Feb 16, 2018

lberki commented Feb 16, 2018 • edited Loading

brandjon commented Mar 14, 2019

Make init.py files creation optional #4470

Make init.py files creation optional #4470

mouadino commented Jan 17, 2018 •

edited

Loading

mouadino Jan 17, 2018 •

edited

Loading

lberki commented Jan 26, 2018 •

edited

Loading

lberki commented Jan 30, 2018 •

edited

Loading

lberki commented Feb 14, 2018 •

edited

Loading

lberki commented Feb 14, 2018 •

edited

Loading

lberki commented Feb 16, 2018 •

edited

Loading