[AIC-py] cli rage#870
Conversation
jonathanlastmileai
requested review from
Ankush-lastmile,
rholinshead,
rossdanlm,
saqadri and
suyoglastmileai
as code owners
jonathanlastmileai
force-pushed
the
pr870
branch
from
3f75f6b to
dd6baa7
Compare
| except Exception as e: | ||
| return core_utils.ErrWithTraceback(e) | ||
|
|
||
| cli_config_object = core_utils.read_text_file(state.cli_config_path).and_then(_yaml_deserialize) |
There was a problem hiding this comment.
Cool is and_then another python quirk?
There was a problem hiding this comment.
nope, it's a Result quirk :)
https://github.com/rustedpy/result
In turn it's basically borrowed from Haskell. Some of the theory
There was a problem hiding this comment.
that link 404s.
| for edit_config in res_edit_config | ||
| for cli_config in res_cli_config | ||
| for res_servers_ok in _run_editor_servers(edit_config, cli_config.config_path) |
There was a problem hiding this comment.
What does this do? This goes with my earlier comment about variable names not being clear and intuitive to understand
There was a problem hiding this comment.
This is admittedly arcane syntax that unpacks the result of _run_editor_servers (if it's an Ok). You can think of it kind of like a Result comprehension, sort of like a list comprehension that flattens a 2d list.
Does that answer the question? What would you name differently?
There was a problem hiding this comment.
See do notation in the readme: https://github.com/rustedpy/result
There was a problem hiding this comment.
Let me take a look and read to understand, I've never seen 3 for loops list comprehension within the same inline (still not super familiar how inline tabs work in Python tbh, so it's just not clear to me what parts are nested or not)
There was a problem hiding this comment.
Comprehensions with more than one for are indeed confusing. I was confused about this piece of plain ordinary Python for many years:
Surprisingly, learning about the equivalent thing for Result, which as I mentioned is very FP and unpythonic, made me understand this Python quirk better.
There was a problem hiding this comment.
Oh thanks, this example makes it super clear to me!
|
|
||
| results: list[Result[str, str]] = [] | ||
| backend_res = run_backend_server(edit_config) | ||
| backend_res = run_backend_server(edit_config, cli_config_path) |
There was a problem hiding this comment.
nit: I really do not like using res. Can we pls use response or result so it's clear which it is?
There was a problem hiding this comment.
Sure, actually we should pick a convention that doesn't do the "maybe_..." thing that @rholinshead pointed out.
One suggestion:
backend_server_outcome = ...
match backend_server_outcome:
Ok(backend_server_outcome_ok)
...
Err(e)
...
| print("Do you know how many Zuckerbucks I should be charging for this?") | ||
| rage.spin(5) | ||
| print("I'm glad we're finally spending time together.") | ||
| rage.spin(4, type="music") |
There was a problem hiding this comment.
Does this end up repeating after we get to the end?
There was a problem hiding this comment.
Repeating what?
There was a problem hiding this comment.
Ohhhhhh, so we're just like spending 20s trolling the people who are upset and want to rage shake? We're not actually "waiting" on anything? lmao ok uh.... hmmmmmmmm, I don't know if that's the best user experience. Either way I think we can remove if we really want to and just get this unblocked for now
There was a problem hiding this comment.
Counterintuitively, I actually think this might be better UX than just doing it. I really think rage might be mostly bedside manner and a little engineering.
| print("I'm glad we're finally spending time together.") | ||
| rage.spin(4, type="music") | ||
|
|
||
| res_rage_config = core_utils.parse_args(main_parser, argv[1:], rage.RageConfig) |
There was a problem hiding this comment.
Ok so this adds the "rage" arg to the rage.RageConfig object? https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.parse_args
How does this work? Does this mean a new field/property is created for this class (and all objects that now instantiate rage.RageConfig now possess this field)?
There was a problem hiding this comment.
This is a handy wrapper that uses argparse under the hood to return a struct directly.
It can be made a little clearer: e.g. we have to manually give argv[1:] and specify the config type twice, once inside subparser_record_types and once in parse_args().
| rage.spin(4, type="music") | ||
|
|
||
| res_rage_config = core_utils.parse_args(main_parser, argv[1:], rage.RageConfig) | ||
| res_rage = res_rage_config.and_then(rage.rage) |
There was a problem hiding this comment.
Why are we making rage.rage() a class method which requires us to create a res_rage_config instead of using a static method? Is it becuase we want to re-use state/object in case user calls multiple rages in the same session?
There was a problem hiding this comment.
we want to re-use state/object
I would never do such a thing - how dare you.
it's just a module with a function. rage module has rage function.
def rage(config: RageConfig) -> Result[None, str]:
| logger.setLevel(config.log_level) | ||
| print("\n\n\n\n............\n\n") | ||
| print("Please open an issue! :) Here are some tips on how to do that:") | ||
| for logfile in ["editor_flask_server.log", "aiconfig.log"]: |
There was a problem hiding this comment.
Why do we need to do two logfiles? I think this could be confusing to users having to submit 2 separate logfiles + issues
There was a problem hiding this comment.
Would be better to merge logfiles together if possible
There was a problem hiding this comment.
Agreed, but that should be a follow up. This is enough to get us started
| print("\nPlease run the following commands and also include their output:") | ||
| print("\nwhich pip; which pip3; which python; which python3; pip3 list | grep aiconfig; python --version; python3 --version") |
There was a problem hiding this comment.
Can be followup diff, but think we should do this ourselves and add to one of the logfiles
There was a problem hiding this comment.
We could, but it's really easy for the user to just copy/paste this one-liner. I don't want to create room for new bugs during the flow for reporting bugs. It should be simple reliable and predictable.
There was a problem hiding this comment.
I actually agree now and I did a lot of this in #903
jonathanlastmileai
force-pushed
the
pr870
branch
from
dd6baa7 to
fa39199
Compare
|
|
||
|
|
||
| class RageConfig(core_utils.Record): | ||
| log_level: str | int = "WARNING" |
There was a problem hiding this comment.
Do we need this? I don't see it being used
Or is it an abstract field we need to override for core_utils.Record?
There was a problem hiding this comment.
I'm going to use this in a subsequent PR. It's nice to have every subcommand have a controllable logging level.
There was a problem hiding this comment.
Can we move it to subsequent PR then until it's needed? Keeps things easier to logic together and review
There was a problem hiding this comment.
If you want we can keep the line just commented out
There was a problem hiding this comment.
Wait, I am using it
jonathanlastmileai
force-pushed
the
pr870
branch
from
fa39199 to
cd87b4b
Compare
This just tells the user how to open an issue. I don't want to do anything automatically because (a) user trust, and (b) it's much harder to implement without additional bugs. This is mostly bedside manner. If the user is raging, they are angry. Troll them a little bit to lighten the mood first. https://github.com/lastmile-ai/aiconfig/assets/148090348/2369306f-3af7-49f5-96bb-0ac9be323b51
jonathanlastmileai
force-pushed
the
pr870
branch
from
cd87b4b to
0f0182a
Compare
| spin(4, type="music") | ||
| print("Turning up the heat...") | ||
| spin(2) | ||
| print("If I had a dollar for every time I've seen this error...") |
There was a problem hiding this comment.
Just wanted to mention I love this kind of playfulness. I hope we do more of this in the future! It brings a bit of personality and a piece of ourselves in the products we build. Really nice idea @jonathanlastmileai !
There was a problem hiding this comment.
Glad you like it! I felt like this was a risk. I volunteer to serve as the LastMile Secretary of Keeping it Real.
|
[AIC-py] cli rage part 2: issue draft Try to do a lot of stuff automatically. Run it and you'll see exactly what I mean :) `aiconfig rage` <img width="503" alt="Screenshot 2024-01-12 at 1 33 48 PM" src="https://github.com/lastmile-ai/aiconfig/assets/148090348/0415e458-681d-446f-b8ed-91b7a7c4bf1b"> --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/903). * __->__ #903 * #870 * #869
Super simple for now. Tried at first to connect to Jonathan's rage functiona but that was defined in the editor package: #870 ## Test Plan
Super simple for now. Tried at first to connect to Jonathan's rage functiona but that was defined in the editor package: #870 ## Test Plan https://github.com/lastmile-ai/aiconfig/assets/151060367/1ab905d0-dec8-4168-9a4c-995b7af0cd28
…ack (#1362) [ez] Redirect users to our issues creation page when submitting feedback Super simple for now. Tried at first to connect to Jonathan's rage functiona but that was defined in the editor package: #870 ## Test Plan https://github.com/lastmile-ai/aiconfig/assets/151060367/1ab905d0-dec8-4168-9a4c-995b7af0cd28 --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/1362). * __->__ #1362 * #1360
3 participants
[AIC-py] cli rage
This just tells the user how to open an issue. I don't want to do anything automatically
because (a) user trust, and (b) it's much harder to implement without additional bugs.
This is mostly bedside manner. If the user is raging, they are angry.
Troll them a little bit to lighten the mood first.
rage2.mov
Stack created with Sapling. Best reviewed with ReviewStack.