Skip to content

Remove local flag. #63

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

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Remove local flag. #63

merged 1 commit into from
Nov 29, 2023

Conversation

8W9aG
Copy link
Contributor

@8W9aG 8W9aG commented Nov 27, 2023

  • Local always results in true.
  • Unclear why the flag is useful in this case.
  • Currently impedes builds using bazel RBEs.

* Local always results in true.
* Unclear why the flag is useful in this case.
* Currently impedes builds using bazel RBEs.
@rwgk
Copy link
Collaborator

rwgk commented Nov 27, 2023

This looks like it undoes #29, which had quite a bit of discussion. I expect that merging this PR will break what was fixed by #29.

Tagging everybody who commented on #29: Could you please advise?@quval, @SomeoneSerge, @TheButlah, @jordanzliu, @dmadisetti, @dstripelis, @QuantamHD, @chuckx

@8W9aG on #62 you wrote:

Having said that local is always true, so maybe it just needs to be propagated?

I believe that would be much safer.

@quval
Copy link
Contributor

quval commented Nov 27, 2023

If I remember correctly, what #29 actually did (among the rest) was adding the option to disable local execution. I'm not using pybind11_bazel's python_configure.bzl any longer, but in general I'm all for removing local = True.

@8W9aG
Copy link
Contributor Author

8W9aG commented Nov 29, 2023

I am fine with doing either (removing or bringing the option up to the main rule).

If I could put my "what to do" hat on for a second though, I think its a bit strange there is a local force and we don't know why it exists. It seems to me like this is a leftover from a debugging session that somehow made its way into production code. I think it might be better to remove it and wait for something to break, and if so re-enable it, and leave a comment as to why this exists. I suppose I think that even if we go with the propagation method, people might be confused when this rule doesn't work for RBEs, and its not immediately obvious that they will need to manually switch a flag on the rule (they will assume this is done by the bazel settings implicitly).

Having said that, it does introduce a risk for the maintainers as potentially breaking a production component, so I understand if that doesn't want to be taken. Happy to go with either direction.

@rwgk
Copy link
Collaborator

rwgk commented Nov 29, 2023

I think it might be better to remove it and wait for something to break, and if so re-enable it,

OK, sounds good. Let's just remove it for now.

If it's needed by anyone later, maybe it's better to propagate tags?

From the docs:

local | Boolean; default is False

If set to True, this option forces this genrule to run using the "local" strategy, which means no remote execution, no sandboxing, no persistent workers.

This is equivalent to providing 'local' as a tag (tags=["local"]).

@rwgk rwgk merged commit 23926b0 into pybind:master Nov 29, 2023
@rwgk
Copy link
Collaborator

rwgk commented Nov 29, 2023

For the record: testing with pybind11_abseil and pybind11_protobuf passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants