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

Airbyte-ci: Add ClickPipelineContext and update test to use #31628

Merged
merged 36 commits into from
Nov 7, 2023

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented Oct 19, 2023

Overview

  1. Adds the ClickPipelineContext to give developers a primitive to merge click commands, dagger clients and pipeline contexts into one
  2. Updates airbyte-ci test to use ClickPipelineContext
  3. Adds new click decorators for automatically attaching click arguments to the click context object so that child commands may use them

@vercel
Copy link

vercel bot commented Oct 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2023 6:14pm

@track_command
@click_track_command
@click_merge_args_into_context_obj
@click_append_to_context_object("is_ci", lambda ctx: not ctx.obj["is_local"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alafanechere @erohmensing Added a new decorator to standardize appending to the context, and make it closer to argument/option decorators.

basically replaces

ctx.obj["is_local"] = value

Not super attached but felt useful

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it!

Copy link
Contributor

Choose a reason for hiding this comment

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

❓ In the case that setting the parameter is more complicated than just setting a value, would you suggest adding a util method and referencing it here? or doing it in the method itself?

I would potentially be looking into making this accept a coroutine in addition to a callable or value, if it fixes my problems with trying to append something. But that doesn't have to be done here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! Would definately recommend a named function instead of a lambda.

Also would welcome a coroutine version! Ill look into updating it to allow that

@click.pass_context
@track_command
@click_track_command
@click_merge_args_into_context_obj
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alafanechere @erohmensing This is what merges all click arguments/options into the context object by default.

basically another name would be @pass_all_click_args_to_child

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this in use before I got down to my comment. This is sexy 🔥

from pipelines.cli.click_decorators import LazyPassDecorator, click_ignore_unused_kwargs, click_merge_args_into_context_obj
from pipelines.models.contexts.click_pipeline_context import ClickPipelineContext

pass_pipeline_context: LazyPassDecorator = LazyPassDecorator(ClickPipelineContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alafanechere @erohmensing This is the new kid on the block for instantiating a pipeline/dagger client

@bnchrch bnchrch marked this pull request as ready for review October 24, 2023 19:29
@bnchrch bnchrch requested a review from a team October 24, 2023 19:29
@bnchrch bnchrch changed the title Draft: ClickPipelineContext Airbyte-ci: Add ClickPipelineContext and update test to use Oct 24, 2023
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

I love the direction! You're becoming a decorator wizard!
My main concerns are the following:

  1. Some command options end up in the command function signature, some others don't. I'd love to have a strict command signature policy. I'd be fine if all the options ended up being merged in the context obj.

  2. You wrote Adds the ClickPipelineContext to give developers a primitive to merge click commands, dagger clients and pipeline contexts into one . I don't see how click context gets merged with the current PipelieContext class. So far the benefits I've grasped are:

  • less boilerplate code to load option values to the context.
  • A singleton dagger client

Let me know if I missed something!

Comment on lines 195 to 231
@click_merge_args_into_context_obj
@click_ignore_unused_kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it awkward that some options end up being part of the function signature and others don't.
Can we "simply" remove all function args except ctx and is ctx access in the body of the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup! can do!

from pipelines.helpers.utils import sh_dash_c
from pipelines.models.contexts.click_pipeline_context import ClickPipelineContext

pass_pipeline_context: LazyPassDecorator = LazyPassDecorator(ClickPipelineContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we pass_pipeline_context click_pipeline_context module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure I follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, missings words.
Can we from pipelines.models.contexts.click_pipeline_context import pass_pipeline_context

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, since pretty much everywhere will be using this

main_logger.info(f"Modified Files: {ctx.obj['modified_files']}")


def _get_gha_workflow_run_id(ctx: click.Context) -> Optional[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need such a getter? Isn't ctx.obj.get("gha_workflow_run_id") sufficient?

return ctx.obj["gha_workflow_run_id"]


def _get_pull_request(ctx: click.Context):
Copy link
Contributor

Choose a reason for hiding this comment

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

A type hint on the return would be nice

@track_command
@click_track_command
@click_merge_args_into_context_obj
@click_append_to_context_object("is_ci", lambda ctx: not ctx.obj["is_local"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Love it!

Comment on lines +32 to +38
class Config:
arbitrary_types_allowed = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we declaring a nested class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class was lifted from Aircmd with minor modifications. Didnt want to mess with an implementation that was working

@alafanechere
Copy link
Contributor

Some command options end up in the command function signature, some others don't. I'd love to have a strict command signature policy. I'd be fine if all the options ended up being merged in the context obj.

@alafanechere I dont think this is achievable without extending / wrapping @click.command Which I think is a bit too big of a lift to restrict use.

Even if we don't restrict use, can we make sure that we follow this policy? In other words, in this PR can you avoid passing the options to the signature and just pull their values from the context? Or is it something click forces you to do?

You wrote Adds the ClickPipelineContext to give developers a primitive to merge click commands, dagger clients and pipeline contexts into one . I don't see how click context gets merged with the current PipelineContext class.

Ah, test may not be the best example but if you look here

@click.command()
@click.argument("poetry_package_path")
@click.option("--test-directory", default="tests", help="The directory containing the tests to run.")
@pass_pipeline_context
@click_ignore_unused_kwargs
async def test(
    pipeline_context: ClickPipelineContext,
    poetry_package_path: str,
    test_directory: str,
):

pipeline_context is the only context object necessary.

  1. You can get the instantiated dagger client via pipeline_context.get_dagger_client()
  2. You can get the click context via pipeline_context._click_context
  3. You can access click params and object via pipeline_context.params

@bnchrch yep it's nice.
My question was rather "how easy is it now to avoid instantiating ConnectorContext with such a bulky approach?"

@octavia-squidington-iii octavia-squidington-iii removed area/connectors Connector related issues CDK Connector Development Kit labels Nov 7, 2023
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Nothing blocking! Thank you for the effort!
Please run a pre-release + connector test manually before merging 🙏


_dagger_client_lock: anyio.Lock = PrivateAttr(default_factory=anyio.Lock)

async def get_dagger_client(self, client: Optional[Client] = None, pipeline_name: Optional[str] = None) -> Client:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to have a public PipelineContext.dagger_client attribute / property.
It will be more convenient than running await pipeline_context.get_dagger_client()
Could we expose an async init class method to initialize PipelineContext where the _dagger_client attribute would be set?

pipeline_context = await PipelineContext.init(data)
pipeline_context.dagger_client

The benefit of get_dagger_client is that you can pass custom pipeline name though.
Let me know your opinion about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Closing the loop:

I left a reply in slack on this but TLDR I dont think the gains of avoiding the async call here are worth it.

The Reply:

So If we cant await in the constructor

Then we would have to explicitly call
pipeline_context = await PipelineContext.init(data)
somewhere in the command hierarchy

which has a few downsides (as far as im understanding it)

  • extra boilerplate
  • an extra thing to remember to run
  • that extra thing may be called well before the area of the code your working on (i.e. in the group and your working on a subcommand) leaving you unaware of its existence or necessity
  • and an extra piece to consider when chaining together subcommands

The upside is we dont use await

Im not sure thats worth the speed trade off

Though Im not sure the speed price were paying


_dagger_client_lock: anyio.Lock = PrivateAttr(default_factory=anyio.Lock)

async def get_dagger_client(self, client: Optional[Client] = None, pipeline_name: Optional[str] = None) -> Client:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async def get_dagger_client(self, client: Optional[Client] = None, pipeline_name: Optional[str] = None) -> Client:
async def get_dagger_client(self, pipeline_name: Optional[str] = None) -> Client:

I don't think this client parameter is used in your function.

@bnchrch
Copy link
Contributor Author

bnchrch commented Nov 7, 2023

  • Tests are passing
  • Connector Prerelease was successful
  • Connector test was successful

@bnchrch bnchrch merged commit 22fc00a into master Nov 7, 2023
21 checks passed
@bnchrch bnchrch deleted the bnchrch/simple-pipeline-context branch November 7, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants