Improve execution name readability #5637

wayner0628 · 2024-08-06T13:23:29Z

Tracking issue

Why are the changes needed?

Improve execution name readability

What changes were proposed in this pull request?

Add EnableHumanHash config in flyteadmin/pkg/runtime/interfaces/application_configuration.go
Add human hash feature in flyteadmin/pkg/common/executions.go

How was this patch tested?

unit testing and integration testing

Setup process

To enable human-readable execution names, add the following configuration:

flyteadmin:
  featureGates:
    enableFriendlyNames: true

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

flyteorg/flytekit#2678

Docs link

Signed-off-by: wayner0628 <a901639@gmail.com>

welcome · 2024-08-06T13:23:32Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

Signed-off-by: wayner0628 <a901639@gmail.com>

codecov · 2024-08-06T17:17:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.17%. Comparing base (5c147ef) to head (f2b7b7f).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5637      +/-   ##
==========================================
- Coverage   36.17%   36.17%   -0.01%     
==========================================
  Files        1302     1303       +1     
  Lines      109614   109611       -3     
==========================================
- Hits        39653    39649       -4     
- Misses      65816    65819       +3     
+ Partials     4145     4143       -2

Flag	Coverage Δ
unittests-datacatalog	`51.37% <ø> (ø)`
unittests-flyteadmin	`55.28% <100.00%> (-0.02%)`	⬇️
unittests-flytecopilot	`12.17% <ø> (ø)`
unittests-flytectl	`62.18% <ø> (ø)`
unittests-flyteidl	`7.12% <ø> (ø)`
unittests-flyteplugins	`53.34% <ø> (ø)`
unittests-flytepropeller	`41.71% <ø> (ø)`
unittests-flytestdlib	`55.35% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: wayner0628 <a901639@gmail.com>

pingsutw · 2024-08-12T05:57:57Z

flytectl/go.mod

@@ -15,6 +15,7 @@ require (
 	github.com/docker/docker v26.1.5+incompatible
 	github.com/docker/go-connections v0.4.0
 	github.com/enescakir/emoji v1.0.0
+	github.com/flyteorg/flyte/flyteadmin v0.0.0-00010101000000-000000000000


we should not import flyteadmin here

flytectl/cmd/create/execution_util.go

flyteadmin/pkg/common/executions.go

Signed-off-by: wayner0628 <a901639@gmail.com>

flyteadmin/pkg/async/schedule/aws/workflow_executor.go

flyteadmin/go.mod

flyteadmin/pkg/async/schedule/aws/workflow_executor.go

flyteadmin/pkg/common/executions.go

flyteadmin/pkg/manager/impl/util/shared.go

flyteadmin/pkg/runtime/interfaces/application_configuration.go

eapolinario · 2024-08-22T22:10:54Z

flyteadmin/scheduler/executor/executor_impl.go

+	executionName := ""
+	config := runtime.NewApplicationConfigurationProvider()
+	if config.GetTopLevelConfig().FeatureGates.EnableHumanHash {
+		executionName, err = common.GetExecutionName(time.Now().UnixNano(), config.GetTopLevelConfig().FeatureGates.EnableHumanHash)
+		if err != nil {
+			logger.Errorf(ctx, "failed to generate execution name for schedule %+v due to %v", s, err)
+			return err
+		}
+	} else {
+		executionName = "f" + strings.ReplaceAll(executionIdentifier.String(), "-", "")[:19]
+	}


Are friendly names important for scheduled executions?

flyteadmin/pkg/common/executions.go

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 · 2024-08-26T05:49:50Z

Hello @pingsutw @eapolinario,

I've made the following changes based on your feedback:

Removed all unnecessary error handlers.
Added a comment explaining why errors in humanhash generation can be safely ignored.
Updated the humanhash package version to v1.1.0.
Renamed the configuration setting to EnableFriendlyNames.
Extend the name to 4 subwords.
Now reading the config directly within the getExecutionName function, and adjusted the file path accordingly.
Updated the unit tests to use mocking objects, as the config is now read within the function.

Please review these changes and let me know if you have any suggestions or concerns.

Thank you!

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 · 2024-08-27T11:07:13Z

Hi @pingsutw @eapolinario,

I've thoroughly investigated the issue but couldn't find any connection between my changes and the CI unit test failure. Additionally, there are no error messages logged that could provide further insight. Could you please help retrigger the CI build? Thank you!

Signed-off-by: Kevin Su <pingsutw@apache.org>

…hash-execution-id

Signed-off-by: Kevin Su <pingsutw@apache.org>

pingsutw

I just reverted the flytectl changes. It seems like we can't change that in the same PR. You could create a follow-up PR to update flytectl.

wayner0628 · 2024-08-29T08:07:09Z

I just reverted the flytectl changes. It seems like we can't change that in the same PR. You could create a follow-up PR to update flytectl.

I noticed some issues still persist, so I'll continue checking them.

welcome · 2024-08-30T17:49:44Z

Congrats on merging your first pull request! 🎉

ddl-ebrown · 2024-09-03T20:16:16Z

FYI - we implemented something similar in our custom auth gateway, as we weren't sure if this would be something Flyte would use.

A few differences with our implementation:

We use https://github.com/dustinkirkland/golang-petname for the names
To create enough uniqueness, we use 3 terms PLUS a 4 character suffix (we wanted execution names to be unique across all of Flyte, not just within a specific project / domain). The suffix is produced using https://github.com/matoous/go-nanoid
- Humanhash looks to be using a similar or the same word list as golang-petname. 3 words in golang-petname is about 53 million combinations, which we didn't think was enough uniqueness. With the 4 character suffix, we end up with about 89 trillion combinations.

We would consider removing our implementation in favor of upstream if we can get a better uniqueness guarantee. Would you be willing to modify the friendly naming here to support that?

We get names like hardly-maximum-sculpin-rax1, commonly-maximum-narwhal-jubn, officially-pure-alien-tm9a , etc

pingsutw · 2024-09-03T21:06:44Z

@ddl-ebrown, we could make suffix opt-in. Need to add one more field to the config map. something like

flyteadmin
  featureGates:
    enableFriendlyNames: true
  execution_name_with_random_suffix: true

Feel free to open a PR!

kumare3 · 2024-09-05T03:09:20Z

This is pretty cool - cc @katrogan

Signed-off-by: wayner0628 <a901639@gmail.com> Signed-off-by: Kevin Su <pingsutw@apache.org> Co-authored-by: Kevin Su <pingsutw@apache.org> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

This reverts commit 2ed2408.

Signed-off-by: wayner0628 <a901639@gmail.com> Signed-off-by: Kevin Su <pingsutw@apache.org> Co-authored-by: Kevin Su <pingsutw@apache.org> Signed-off-by: Bugra Gedik <bgedik@gmail.com>

* Revert "fix: Use deterministic execution names in scheduler (#5724)" This reverts commit a058fd1. * Revert "Improve execution name readability (#5637)" This reverts commit 2ed2408. * nit Signed-off-by: Kevin Su <pingsutw@apache.org> * make helm Signed-off-by: Kevin Su <pingsutw@apache.org> --------- Signed-off-by: Kevin Su <pingsutw@apache.org>

wayner0628 added 4 commits August 6, 2024 21:03

feat: humanhash execution id

815f597

Signed-off-by: wayner0628 <a901639@gmail.com>

feat: humanhash config

5f37b11

Signed-off-by: wayner0628 <a901639@gmail.com>

chore: add package

45ce982

Signed-off-by: wayner0628 <a901639@gmail.com>

deprecate: generate execution name later

af06798

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: replace space with tab

c647718

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 added 6 commits August 7, 2024 02:10

fix: gci format check fail

0447a7f

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: generate execution name in workflowExecutor

21f08e1

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: get global config

65e8aa7

Signed-off-by: wayner0628 <a901639@gmail.com>

chore: add package in flytectl

265024d

Signed-off-by: wayner0628 <a901639@gmail.com>

feat: enable humanhash for cmd and scheduler

ac5d5fe

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: set execution name limit to k8s limit

cba751d

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 marked this pull request as ready for review August 11, 2024 14:00

wayner0628 changed the title ~~Create exec id through humanhash~~ Improve execution name readability Aug 11, 2024

pingsutw reviewed Aug 12, 2024

View reviewed changes

wayner0628 added 4 commits August 12, 2024 23:22

fix: revert flytectl change

33fc45b

Signed-off-by: wayner0628 <a901639@gmail.com>

refactor: let flyte backend generate ID

058baf7

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: handle generate name error

bb14487

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: remove unused variables

18d898c

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 mentioned this pull request Aug 12, 2024

Default execution name should be generated in flyteadmin flyteorg/flytekit#2678

Merged

3 tasks

wayner0628 added 3 commits August 14, 2024 03:07

fix: set execution name empty is not specified

a065a58

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: correct import format

910bb65

Signed-off-by: wayner0628 <a901639@gmail.com>

fix: fix lint error

d6f9edb

Signed-off-by: wayner0628 <a901639@gmail.com>

pingsutw reviewed Aug 17, 2024

View reviewed changes

flyteadmin/pkg/async/schedule/aws/workflow_executor.go Outdated Show resolved Hide resolved

flyteadmin/pkg/async/schedule/aws/workflow_executor.go Outdated Show resolved Hide resolved

eapolinario reviewed Aug 22, 2024

View reviewed changes

eapolinario reviewed Aug 23, 2024

View reviewed changes

flyteadmin/pkg/common/executions.go Outdated Show resolved Hide resolved

wayner0628 added 3 commits August 26, 2024 13:33

update humanhash version

5f7151e

Signed-off-by: wayner0628 <a901639@gmail.com>

remove error handler

a3b62bd

Signed-off-by: wayner0628 <a901639@gmail.com>

read config inside get exec name and fix import cycle

2553ec0

Signed-off-by: wayner0628 <a901639@gmail.com>

change configuration name

9646272

Signed-off-by: wayner0628 <a901639@gmail.com>

wayner0628 requested review from pingsutw and eapolinario August 26, 2024 05:53

fix lint error

c138a79

Signed-off-by: wayner0628 <a901639@gmail.com>

pingsutw added 5 commits August 28, 2024 13:34

nit

92f7b9c

Signed-off-by: Kevin Su <pingsutw@apache.org>

nit

5c0dccf

Signed-off-by: Kevin Su <pingsutw@apache.org>

Merge branch 'master' of github.com:flyteorg/flyte into feature/human…

9eb67f5

…hash-execution-id

revert flytectl

d353c16

Signed-off-by: Kevin Su <pingsutw@apache.org>

lint

f2b7b7f

Signed-off-by: Kevin Su <pingsutw@apache.org>

pingsutw approved these changes Aug 28, 2024

View reviewed changes

pingsutw approved these changes Aug 30, 2024

View reviewed changes

pingsutw merged commit 2ed2408 into flyteorg:master Aug 30, 2024
50 checks passed

wayner0628 deleted the feature/humanhash-execution-id branch August 31, 2024 15:18

wayner0628 mentioned this pull request Aug 31, 2024

Move default execution name generation to flyteadmin #5714

Merged

3 tasks

pingsutw added a commit that referenced this pull request Sep 11, 2024

Revert "Improve execution name readability (#5637)"

b0d37db

This reverts commit 2ed2408.

eapolinario mentioned this pull request Sep 26, 2024

Revert "Improve execution name readability" #5740

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve execution name readability #5637

Improve execution name readability #5637

wayner0628 commented Aug 6, 2024 •

edited by pingsutw

Loading

welcome bot commented Aug 6, 2024

codecov bot commented Aug 6, 2024 •

edited

Loading

pingsutw Aug 12, 2024

eapolinario Aug 22, 2024

wayner0628 commented Aug 26, 2024

wayner0628 commented Aug 27, 2024

pingsutw left a comment

wayner0628 commented Aug 29, 2024

welcome bot commented Aug 30, 2024

ddl-ebrown commented Sep 3, 2024 •

edited

Loading

pingsutw commented Sep 3, 2024

kumare3 commented Sep 5, 2024

Improve execution name readability #5637

Improve execution name readability #5637

Conversation

wayner0628 commented Aug 6, 2024 • edited by pingsutw Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

welcome bot commented Aug 6, 2024

codecov bot commented Aug 6, 2024 • edited Loading

Codecov Report

pingsutw Aug 12, 2024

Choose a reason for hiding this comment

eapolinario Aug 22, 2024

Choose a reason for hiding this comment

wayner0628 commented Aug 26, 2024

wayner0628 commented Aug 27, 2024

pingsutw left a comment

Choose a reason for hiding this comment

wayner0628 commented Aug 29, 2024

welcome bot commented Aug 30, 2024

ddl-ebrown commented Sep 3, 2024 • edited Loading

pingsutw commented Sep 3, 2024

kumare3 commented Sep 5, 2024

wayner0628 commented Aug 6, 2024 •

edited by pingsutw

Loading

codecov bot commented Aug 6, 2024 •

edited

Loading

ddl-ebrown commented Sep 3, 2024 •

edited

Loading