Feat/firebase publisher

[deep-diver]

trafficstars

This is a draft PR. Some codes, doc-strings, and comments should be polished. Once they are done, reviews will be requested.

it is now ready. lmk if unit tests are required.

cc: @sayakpaul

[github-actions[bot]]

Thanks for the PR! :rocket:

Instructions: Approve using /lgtm and mark for automatic merge by using /merge.

[casassg]

Btw would love feedback on precommit docs at CONTRIBUTING.md to make sure they make sense

[deep-diver]

Thanks for the comment @casassg :) Will check out the CONTRIBUTING.md, and I will request reviews by today or tomorrow

[deep-diver]

@casassg

should I write unit tests? I have checked it works

[casassg]

Preferably some small unit tests would be great. That helps us do upgrades of dependencies while making sure it's fine. As an example, it helps us bump up what version of tfx is compatible without requiring all contributors to test their components individually for each upgrade.

[deep-diver]

@casassg

some tests are added

[deep-diver]

@sayakpaul addressed your comments :) @casassg ready to have your reviews :D

[sayakpaul]

@deep-diver thanks!

[deep-diver]

addressed your comments @casassg

[casassg]

Seems CI unfortunately still timesout. can you try running locally pip install --dry-run --ignore-installed -e ".[ci_min, firebase_publisher]" This will attempt to resolve the component against our lower support TFX (aka TFX 1.4). ( you may need to upgrade pip - pip install --upgrade pip in order to get --dry-run).

Guessing you may need even a lower version of firebase-admin. It seems to be conflicting with versions of shared google libraries that beam and tensorflow depend on for TFX 1.4

[casassg]

(also if you don't mind can you set the name of the PR to something that describes better the change? We use PR titles as release notes on release)

[deep-diver]

when I run pip install --dry-run --ignore-installed -e ".[ci_min, firebase_publisher]", I get the following error: ERROR: No matching distribution found for tfx<1.10.0,>=1.4.0; extra == "firebase_publisher"

[casassg]

What python version are you in? You may need to reduce the version of python to one that is supported by tfx 1.4

[deep-diver]

I was in Python 3.10, let me try with 3.8

[deep-diver]

I get the same errors with Python 3.6, 3.7, 3.8, 3.9

[casassg]

Okay, I think I found a fix. Change this in version.py

_CI_MIN_CONSTRAINTS = [
    f"tfx~={_INCLUSIVE_MIN_TFX_VERSION}", "tensorflow~=2.6.0",
    "apache-beam[gcp]<2.35", "firebase-admin<5.0.3"
]

The issue is due to https://github.com/firebase/firebase-admin-python/releases/tag/v5.0.3 adding google-api-core 2.X which breaks a lot of stuff w tfx 1.4 and similar as those were released before the 2.X changes. This triggers a tone of backtracking with pip, the fix above just basically tells our CI to bothering looking at any version of firebase_admin above 5.0.3 so it doesnt go down the rabithole of backtracking

[deep-diver]

great! should I fix version.py or are you going to?

[casassg]

you should fix it as part of the PR as otherwise CI wont let you merge it (as CI as of now its failing)

[deep-diver]

I am a little confused, how should I write

_CI_MIN_CONSTRAINTS = [
    f"tfx~={_INCLUSIVE_MIN_TFX_VERSION}", "tensorflow~=2.6.0",
    "apache-beam[gcp]<2.35", "firebase-admin<5.0.3"
]

into version.py ?

since other packages are already defined in _CI_MIN_CONSTRAINTS, I just need like

    "firebase_publisher":
    [f"tfx{_TFXVERSION_CONSTRAINT}", "firebase-admin>=5.0.3,<6.0.0"],

is this right?

[casassg]

Sorry I should have specified more. I meant if you can add "firebase-admin<5.0.3" to https://github.com/tensorflow/tfx-addons/blob/183ee57e8d28edf0252931d9f6b499f719a003da/tfx_addons/version.py#L40

You will need to leave _PKG_METADATA as is but we need modify the CI constraints in order to make sure we can test the component at all

[casassg]

I believe I may also be able to commit to your branch if you prefer me to do that

[deep-diver]

please do

[casassg]

this looks better now :D /lgtm

will let you merge at your own discretion (just comment the merge command to merge that should merge the code)

[github-actions[bot]]

Approval received from @casassg! :white_check_mark:

PR is approved. Missing merge command to auto-merge PR!

[deep-diver]

/merge

[deep-diver]

oh it looks like there is a pending review by @hanneshapke I will wait until he checks this out then. Huge thanks @casassg

[github-actions[bot]]

Merged with approvals from casassg - thanks for the contribution! :tada: