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

build(bazel): introduce tflm_cc_* macros, refactoring away micro_copts #2765

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

rkuester
Copy link
Contributor

Remove micro_copts() by replacing every cc_* target that used
them with a tflm_cc_* equivalent, and setting those common copts
in one place, inside the tflm_cc_* macro.

This is the first of several commits introducing tflm_cc_* macros
in place of cc_binary, cc_library, and cc_test. Motivated by the
upcoming need to support conditional compilation, the objective
is to centralize build configuration rather than requiring (and
remembering that) each cc_* target in the project add the same
common attributes such as compiler options and select()ed

Alternatives such as setting global options on the command line
or in .bazelrc, even if simplified with a --config option, fail
to preserve flags and hooks for configuration in the case TFLM is
used as an external repository by an application project. Nor is
it easy in that case for individual targets to override an
otherwise global setting.

BUG=#2636

Remove micro_copts() by replacing every cc_* target that used
them with a tflm_cc_* equivalent, and setting those common copts
in one place, inside the tflm_cc_* macro.

This is the first of several commits introducing tflm_cc_* macros
in place of cc_binary, cc_library, and cc_test. Motivated by the
upcoming need to support conditional compilation, the objective
is to centralize build configuration rather than requiring (and
remembering that) each cc_* target in the project add the same
common attributes such as compiler options and select()ed

Alternatives such as setting global options on the command line
or in .bazelrc, even if simplified with a --config option, fail
to preserve flags and hooks for configuration in the case TFLM is
used as an external repository by an application project. Nor is
it easy in that case for individual targets to override an
otherwise global setting.

BUG=tensorflow#2636
@rkuester rkuester requested a review from a team as a code owner November 13, 2024 19:57
@suleshahid
Copy link
Collaborator

Could you expand on the conditional compilations? Will we have different tflm_copts and the idea is the user will just change that rather than change all micro_copts?

@rkuester
Copy link
Contributor Author

Could you expand on the conditional compilations? Will we have different tflm_copts and the idea is the user will just change that rather than change all micro_copts?

Yes. Looking ahead, the conditional complication will be implemented through these macros by a change like 884a234. There's a central place to make a change like that once the macros in this commit (tflm_cc_*) are in place.

micro_copts just wasn't general enough—it allowed modification of compiler options, but not modification of other attributes of a target. Plus, a developer had to remember to add micro_copts. The historical inconsistency throughout the codebase confirms that wasn't a reasonable expectation. It's much easier to outlaw naked cc_* targets, and catch those in review or even with tooling.

@suleshahid
Copy link
Collaborator

I see. Unfortunately theres some internal code that use micro_copts as well, which could take some time to change. What's the alternatives, would we just need to add a flag to BUILDs when you want compression like USE_TFLM_COMPRESSION?

@rkuester
Copy link
Contributor Author

I see. Unfortunately theres some internal code that use micro_copts as well, which could take some time to change. What's the alternatives, would we just need to add a flag to BUILDs when you want compression like USE_TFLM_COMPRESSION?

The original micro_copts lives on as tflm_copts (it's named that way to match the naming of the tflm_cc_* macros). I'd suggest the internal code substitute s/micro_copts/tflm_copts throughout. I could resurrect the name micro_copts for internal code to call, but that's ugly.

If this internal code needs to build with compression turned on---yes, ultimately it needs to have USE_TFLM_COMPRESSION defined somehow. If they don't want to adopt our tflm_cc_* macros wholesale, they could use the upcoming tflm_defines() in their define attribute. Or, the ugliest option that might work would be duplication the define in micro_copts maintained for just that internal code.

Add a deprecated alias for tflm_copts(), for the benefit of
code outside the open-source TFLM repository.
@rkuester
Copy link
Contributor Author

After talking in person, we have decided to resurrect micro_copts() as an alias for out-of-tree code.

@suleshahid
Copy link
Collaborator

Do we need to keep tflm_copts then? Can we just keep the name micro_copts as it is and use that.

@rkuester
Copy link
Contributor Author

rkuester commented Nov 14, 2024

Do we need to keep tflm_copts then? Can we just keep the name micro_copts as it is and use that.

I'm inclined to leave them both, because:

  1. Future commits along this feature branch already use tflite_copts().
  2. It's good we maintain a seam along which our internal, TFLM code and the out-of-tree code can differ. Some upcoming code adds to tflite_copts(), and this allows us the option of modifying micro_copts() differently.
  3. The way the defaults can be overridden or augmented in these tflite_cc_* macros is to pass arguments which override the default set of options. So, e.g., you'll see upcoming code like tflm_cc_test(copts = tflm_copts() + ["-foo"], defines = tflm_defines() + ["LOCAL"]). It's nice if all these calls use the same tflm_ prefix.
  4. It's good we make it obvious in the code that micro_copts is deprecated and might diverge from our internal use.

@mergify mergify bot merged commit 11b15b3 into tensorflow:main Nov 14, 2024
28 checks passed
@rkuester rkuester deleted the feat-compression branch November 15, 2024 00:56
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