Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Apr 23, 2022

The cached parameters in Python Breeze were largely based on
Bash implementation. They did the job but required pretty
cumbersome synchronization of cached values with parameters
passed and it was easy to forget about this as you had to
do it sepearately in each method that had potentially
cacheable parameters.

In this PR we take advantage of the Click class hiarchy and their
extendability. We've already extended Click Choice parameter
to be much better formatted for long list of choices but we take
this a bit further now with adding new type of parameter that
can cache the values between runs.

This has multiple advantages:

  • we do not have to remember about synchroniation - parameters
    automatically read/write their values from cache as they are
    used
  • we can automatically set parameters to default when wrong
    value is passed. This is nice as user does not have to
    re-run the command because the values are corrected
    on-the-flight.
  • the parameters can have their default values displayed in
    help screen (we use sentinel default that we can use to
    detect if value was passed from parameter or taken from default)
  • the parameters can also have their values marked in
    list of choices - so the user in the help screen can see not
    only the default but also which value is currently selected
    and will be used if you do not pass any parameter.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Apr 23, 2022

This has been bothered me a bit - our implementation of parameter caching with synchronization was a bit convoluted (mostly because it was based on the Bash implementation).

I took a stab at it and it turned out that we can rather easily do it "properly" i.e. extending Click Choice to make the parameters much "smarter" -now it is enough do declare the parameter as "CacheableChoice" and it does all the between-session caching of last used values- and it even can nicely integrate with the help printed by rich-click in the way that we can see both - default and current values in the --help output"

output-config

@Bowrna -> you might want to take a look and see that this is a really nice improvement :).

@blag -> you might also take some inspiration when convert Airlfow CLI. The Click + Rich Click combination is awesome and wonderfully and easily extensible.

@potiuk potiuk force-pushed the improve-handling-cached-parameters branch from e1d267a to 39e9bad Compare April 23, 2022 23:13
Copy link
Member Author

Choose a reason for hiding this comment

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

Those tests can be deleted now. We completely decoupled caching parameters from creating BuildProdParams, BuildCIParams - by the time they get created, the cache retrieval/writing is already processed via CacheableChoice and those tests make no sense.

@potiuk potiuk requested a review from uranusjr April 23, 2022 23:15
@potiuk potiuk force-pushed the improve-handling-cached-parameters branch from 39e9bad to 330ef05 Compare April 24, 2022 00:07
@potiuk potiuk requested review from kaxil and mik-laj as code owners April 24, 2022 00:07
@potiuk potiuk force-pushed the improve-handling-cached-parameters branch from 330ef05 to 993a0d0 Compare April 24, 2022 21:34
The cached parameters in Python Breeze were largely based on
Bash implementation. They did the job but required pretty
cumbersome synchronization of cached values with parameters
passed and it was easy to forget about this as you had to
do it sepearately in each method that had potentially
cacheable parameters.

In this PR we take advantage of the Click class hiarchy and their
extendability. We've already extended Click Choice parameter
to be much better formatted for long list of choices but we take
this a bit further now with adding new type of parameter that
can cache the values between runs.

This has multiple advantages:

* we do not have to remember about synchroniation - parameters
  automatically read/write their values from cache as they are
  used
* we can automatically set parameters to default when wrong
  value is passed. This is nice as user does not have to
  re-run the command because the values are corrected
  on-the-flight.
* the parameters can have their default values displayed in
  help screen (we use sentinel default that we can use to
  detect if value was passed from parameter or taken from default)
* the parameters can also have their <current> values marked in
  list of choices - so the user in the help screen can see not
  only the default but also which value is currently selected
  and will be used if you do not pass any parameter.
@potiuk potiuk force-pushed the improve-handling-cached-parameters branch from 993a0d0 to fa4866e Compare April 24, 2022 22:33
@potiuk
Copy link
Member Author

potiuk commented Apr 25, 2022

merged as part of #23205

@potiuk
Copy link
Member Author

potiuk commented Apr 25, 2022

Already merged as part of #23205

@potiuk potiuk closed this Apr 25, 2022
@potiuk potiuk deleted the improve-handling-cached-parameters branch July 29, 2022 20:13
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.

1 participant