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

Remove legacy providers #6721

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Conversation

comius
Copy link
Contributor

@comius comius commented Jan 17, 2024

Motivation for features / changes

Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel.

Prerequiste: bazelbuild/rules_closure#599 is merged and released.

Googlers, see cl/597800285.

@yatbear yatbear self-requested a review January 17, 2024 18:59
@yatbear yatbear added type:build/install dependencies Pull requests that update a dependency file labels Jan 17, 2024
arcra added a commit to arcra/tensorboard that referenced this pull request Mar 5, 2024
We haven't updated this dependency on the bazel webtesting rules for a
while, and it looks like we need to do it to be able to also update our
reference to the rules_closure target, to unblock tensorflow#6721.

To be clear, this PR is a prerequisite to pin the rules_closure version
in a separate PR, which in turn will unblock tensorflow#6721.

An attempt to use the versioned browsers provided by the
rules_webtesting dependency was created in tensorflow#6772, but some tests were
failing. It seems that some components or tests were written in a way
that depends on the specific browser (?), so to be able to update this
dependency, this continues to use the same browser version, only
updating the rules we use to define our testing browser, to be
compatible with a newer version of rules_webtesting.
arcra added a commit that referenced this pull request Mar 5, 2024
We haven't updated this dependency on the bazel webtesting rules for a
while, and it looks like we need to do it to be able to also update our
reference to the rules_closure target, to unblock #6721.

To be clear, this PR is a prerequisite to pin the rules_closure version
in a separate PR, which in turn will unblock #6721. (Or at least that's
the expectation. It is yet to be confirmed that the rules_closure
version we want to pin is compatible with this one).

An attempt to use the versioned browsers provided by the
rules_webtesting dependency was created in #6772, but some tests were
failing. It seems that some components or tests were written in a way
that depends on the specific browser (?), so to be able to update this
dependency, this continues to use the same browser version, only
updating the rules we use to define our testing browser, to be
compatible with a newer version of rules_webtesting.
@@ -191,6 +188,11 @@ http_archive(
],
)

# WORKAROUND for rules_webtesting not declaring used com_github_gorilla_mux repo:
Copy link
Member

Choose a reason for hiding this comment

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

Can we move these lines above where the other load statement from rules_webtesting is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I tried. com_github_gorilla_mux needs setup for Go repositories. It doesn't work even when used after rules_closure.

tensorboard/defs/web.bzl Outdated Show resolved Hide resolved
# interfaces ~must be the same.
return [
WebFilesInfo(
manifest = manifest,
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, are both of these expected? manifest and manifests (one plural, one singular)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx.label,
))
new_webpaths.append(webpath)
manifest_srcs.append(struct(
Copy link
Member

Choose a reason for hiding this comment

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

Just checking... I understand this change is removing use of struct (is that right?)

Should I be looking to catch any uses of struct? Or it it specific ones from how they're used by rules_closure, so it's tricky to know which ones are right and which ones aren't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not removing all structs, just the structs returned directly from a rule. This one is not a problem.

strip = ctx.attr.strip_prefix
if strip:
if strip.startswith("/"):
_fail(ctx, "strip_prefix should not end with /")
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing issue, but this should be "should not start with /"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to mix semantic changes into a cleanup cl. Please do the fixes as a followup.

Legacy struct providers have been deprecated by Bazel. Replacing them with modern providers, will make it possible to simplify and remove legacy handling from Bazel.
@comius comius requested a review from arcra March 14, 2024 08:45
Copy link
Member

@yatbear yatbear left a comment

Choose a reason for hiding this comment

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

Thank you so much!!

@arcra arcra merged commit 852af8d into tensorflow:master Mar 14, 2024
13 checks passed
c-mita added a commit to c-mita/tensorboard that referenced this pull request Mar 20, 2024
Reapplies tensorflow#6778 after
tensorflow#6721 inadvertently
reverted part of it.

The "to_proto" method on Starlark structs is deprecated and shouldn't be
used. Instead, the proto module's "encode_text" function should be used.
(https://bazel.build/rules/lib/toplevel/proto)

It, along with "to_json", can be disabled in Bazel using the flag
--incompatible_struct_has_no_methods

The underlying implementation is the same, so there should be no
observable changes in final outputs.
c-mita added a commit to c-mita/tensorboard that referenced this pull request Mar 26, 2024
Reapplies tensorflow#6778 after
tensorflow#6721 inadvertently
reverted part of it.

The "to_proto" method on Starlark structs is deprecated and shouldn't be
used. Instead, the proto module's "encode_text" function should be used.
(https://bazel.build/rules/lib/toplevel/proto)

It, along with "to_json", can be disabled in Bazel using the flag
--incompatible_struct_has_no_methods

The underlying implementation is the same, so there should be no
observable changes in final outputs.
bmd3k pushed a commit that referenced this pull request Mar 26, 2024
Reapplies #6778 after
#6721 inadvertently
reverted part of it.

The "to_proto" method on Starlark structs is deprecated and shouldn't be
used. Instead, the proto module's "encode_text" function should be used.
(https://bazel.build/rules/lib/toplevel/proto)

It, along with "to_json", can be disabled in Bazel using the flag
--incompatible_struct_has_no_methods

The underlying implementation is the same, so there should be no
observable changes in final outputs.
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
We haven't updated this dependency on the bazel webtesting rules for a
while, and it looks like we need to do it to be able to also update our
reference to the rules_closure target, to unblock tensorflow#6721.

To be clear, this PR is a prerequisite to pin the rules_closure version
in a separate PR, which in turn will unblock tensorflow#6721. (Or at least that's
the expectation. It is yet to be confirmed that the rules_closure
version we want to pin is compatible with this one).

An attempt to use the versioned browsers provided by the
rules_webtesting dependency was created in tensorflow#6772, but some tests were
failing. It seems that some components or tests were written in a way
that depends on the specific browser (?), so to be able to update this
dependency, this continues to use the same browser version, only
updating the rules we use to define our testing browser, to be
compatible with a newer version of rules_webtesting.
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
## Motivation for features / changes
Legacy struct providers have been deprecated by Bazel. Replacing them
with modern providers, will make it possible to simplify and remove
legacy handling from Bazel.

Prerequiste: bazelbuild/rules_closure#599 is
merged and released.

Googlers, see cl/597800285.
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
Reapplies tensorflow#6778 after
tensorflow#6721 inadvertently
reverted part of it.

The "to_proto" method on Starlark structs is deprecated and shouldn't be
used. Instead, the proto module's "encode_text" function should be used.
(https://bazel.build/rules/lib/toplevel/proto)

It, along with "to_json", can be disabled in Bazel using the flag
--incompatible_struct_has_no_methods

The underlying implementation is the same, so there should be no
observable changes in final outputs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file type:build/install
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants