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

Add identity to task execution metadata #2315

Merged

Conversation

noahjax
Copy link
Contributor

@noahjax noahjax commented Apr 1, 2024

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

For our specific use case it is critical to be able to determine the identity for the subject who launches a task.

What changes were proposed in this pull request?

Add identity to TaskExecutionMetadata. This builds on top of #2282, and relies on flyteorg/flyte#5105 to add identity to the flyteidl TaskExecutionMetadata.

How was this patch tested?

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 46.81%. Comparing base (133e8d5) to head (bc1efd9).
Report is 89 commits behind head on master.

Files Patch % Lines
flytekit/models/task.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2315       +/-   ##
===========================================
- Coverage   83.50%   46.81%   -36.70%     
===========================================
  Files         324      219      -105     
  Lines       24672    20272     -4400     
  Branches     3512     3600       +88     
===========================================
- Hits        20603     9490    -11113     
- Misses       3438    10694     +7256     
+ Partials      631       88      -543     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@noahjax noahjax marked this pull request as ready for review April 3, 2024 17:35
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Apr 3, 2024
dev-requirements.in Outdated Show resolved Hide resolved
pingsutw
pingsutw previously approved these changes Apr 4, 2024
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Apr 4, 2024
eapolinario
eapolinario previously approved these changes Apr 8, 2024
Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

LGTM, but we can only merge this after we get a new flyteidl release.

@ddl-ebrown
Copy link
Contributor

ddl-ebrown commented May 1, 2024

@eapolinario with the 1.12 beta release cut, can this be merged now?

ddl-ebrown pushed a commit to dominodatalab/flytekit that referenced this pull request May 4, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

Signed-off-by: noahjax <[email protected]>
ddl-ebrown pushed a commit to dominodatalab/flytekit that referenced this pull request May 4, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

 - This PR is based on upstream tag v1.12.0b7

Signed-off-by: noahjax <[email protected]>
@eapolinario
Copy link
Collaborator

@noahjax , now that flyteidl 1.12.0 is out, can you set it as the lower bound in pyproject.toml?

@noahjax
Copy link
Contributor Author

noahjax commented May 8, 2024

I recently left Domino and it seems that I have lost push access to repos in the dominodatalab org, as I was unable to push this version bump. @ddl-ebrown would you be able to help push this across the line 🙏🏽

@ddl-ebrown
Copy link
Contributor

Can do - will work to get this one in tomorrow. Thanks!

@ddl-rliu ddl-rliu dismissed stale reviews from eapolinario and pingsutw via a1ba33e May 10, 2024 22:59
noahjax and others added 5 commits May 10, 2024 16:00
Signed-off-by: noahjax <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: noahjax <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: noahjax <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
@ddl-rliu ddl-rliu force-pushed the noahjax.add-identity-to-metadata branch from a1ba33e to bc1efd9 Compare May 10, 2024 23:00
@ddl-rliu
Copy link
Contributor

@ddl-ebrown
Copy link
Contributor

@eapolinario done!

bc1efd9#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R24

Thanks @ddl-rliu !

ddl-ebrown pushed a commit to dominodatalab/flytekit that referenced this pull request May 16, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

 - This PR is based on upstream tag v1.12.0b7

Signed-off-by: noahjax <[email protected]>
ddl-ebrown pushed a commit to dominodatalab/flytekit that referenced this pull request May 16, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

 - This PR is based on upstream tag v1.12.0b7

Signed-off-by: noahjax <[email protected]>
ddl-rliu pushed a commit to dominodatalab/flytekit that referenced this pull request May 23, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

 - This PR is based on upstream tag v1.12.0b7

Signed-off-by: noahjax <[email protected]>
ddl-rliu pushed a commit to dominodatalab/flytekit that referenced this pull request May 23, 2024
 - This is necessary due to a pending PR to upstream flytekit:
   flyteorg#2315

   This PR hasn't been merged because a new flyteidl release had to be
   cut.

   Upstream flyteorg/flyte produce a new v1.12.0-b0 tag / release that
   includes the necessary flyteidl changes that can now be referenced
   https://github.com/flyteorg/flyte/releases/tag/v1.12.0-b0

 - Once the flytekit PR merges, this custom tag / version of flytekit
   that we've forked will no longer be necessary

 - NOTE: the reference to flyteidl may not be consumed properly as a
   "child" transitive dependency.

   For instance, pip install may work, while pip install may not -
   depending on the toolchain involved.

 - This PR is based on upstream tag v1.12.0b7

Signed-off-by: noahjax <[email protected]>
@pingsutw pingsutw merged commit 8562fd9 into flyteorg:master Jun 14, 2024
45 of 48 checks passed
@ddl-ebrown ddl-ebrown deleted the noahjax.add-identity-to-metadata branch June 14, 2024 19:47
@ddl-ebrown
Copy link
Contributor

YES - thanks @pingsutw !

bgedik pushed a commit to bgedik/flytekit that referenced this pull request Jul 3, 2024
Signed-off-by: noahjax <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Co-authored-by: ddl-rliu <[email protected]>
Signed-off-by: bugra.gedik <[email protected]>
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: noahjax <[email protected]>
Signed-off-by: ddl-rliu <[email protected]>
Co-authored-by: ddl-rliu <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants