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

fix: Add CODE_SNIPPET_USE_AUTO_CONFIG environment variable #883

Merged

Conversation

ddl-rliu
Copy link
Contributor

@ddl-rliu ddl-rliu commented Jul 17, 2024

TL;DR

The inputs/outputs Python code snippet leads to a "Failed to connect" error

Introduce the environment variable CODE_SNIPPET_USE_AUTO_CONFIG, to control whether the config object in the inputs/outputs Python code snippet is automatically constructed i.e. Config.auto(). This will tend to use the admin.endpoint that users can set in their ~/.flyte/config.yaml

It can be left unset to keep the current behavior e.g. uses Config.for_endpoint(window.location.host)

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Users already configure flytekit in ~/.flyte/config.yaml to set admin.endpoint to the correct gRPC endpoint. We can leverage this by using Config.auto() auto config, and building the code snippet around this Config.

Tested fix correctly shows the new code snippet when setting is enabled:
image

Tested fix uses current behavior when setting is not enabled:
image

Tracking Issue

flyteorg/flyte#5578

@ddl-rliu ddl-rliu force-pushed the rliu-add-codesnippeturl-to-config branch 3 times, most recently from 4aa12ed to c08ffc0 Compare July 22, 2024 21:13
@ddl-rliu ddl-rliu changed the title Add CODE_SNIPPET_URL to config fix: Add CODE_SNIPPET_URL to config Jul 22, 2024
@ddl-rliu ddl-rliu changed the title fix: Add CODE_SNIPPET_URL to config fix: Add CODE_SNIPPET_USE_AUTO_CONFIG environment variable Jul 22, 2024
@ddl-rliu ddl-rliu marked this pull request as ready for review July 22, 2024 21:42
@ddl-rliu ddl-rliu force-pushed the rliu-add-codesnippeturl-to-config branch 3 times, most recently from f18322e to a4f809f Compare July 22, 2024 22:56
const config =
env.CODE_SNIPPET_USE_AUTO_CONFIG === "true"
? 'Config.auto()'
: isHttps

Choose a reason for hiding this comment

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

Could you just pass isHttps in as the 2nd parameter to Config.for_endpoint?

Copy link
Contributor Author

@ddl-rliu ddl-rliu Jul 22, 2024

Choose a reason for hiding this comment

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

For context, this follows the nested ternary pattern that I've seen e.g. here

const typeOVerride =
// eslint-disable-next-line no-nested-ternary
isStaticGraph === true
? dTypes.staticNode
: isUnFetchedDynamicNode(node)
? dTypes.nestedMaxDepth
: undefined;

Since python booleans aren't one-to-one with JS booleans, to pass in isHttps as a param it would have to look like

const insecure = isHttps ? "False" : "True"
 ... `Config.for_endpoint("${window.location.host}",${insecure})`

which I wasn't sure would be much clearer.

Copy link

@ddl-ebrown ddl-ebrown Jul 23, 2024

Choose a reason for hiding this comment

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

Yeah, the reason I asked was for exactly the reason the ignored eslinter calls out
https://eslint.org/docs/latest/rules/no-nested-ternary

It's fine to use the existing style / pattern they have here -- when in Rome :)

Copy link
Contributor Author

@ddl-rliu ddl-rliu Jul 23, 2024

Choose a reason for hiding this comment

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

Gotcha! Okay, updated it here: df63f99

Signed-off-by: ddl-rliu <richard.liu@dominodatalab.com>
Signed-off-by: ddl-rliu <richard.liu@dominodatalab.com>
@ddl-rliu ddl-rliu force-pushed the rliu-add-codesnippeturl-to-config branch from df63f99 to 93aec03 Compare July 25, 2024 19:01
@eapolinario eapolinario merged commit 8b0d3cc into flyteorg:master Jul 25, 2024
10 checks passed
@flyte-bot
Copy link
Collaborator

🎉 This PR is included in version 1.17.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants