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

Wasp ai cli mage #1601

Merged
merged 20 commits into from
Dec 22, 2023
Merged

Wasp ai cli mage #1601

merged 20 commits into from
Dec 22, 2023

Conversation

Martinsos
Copy link
Member

@Martinsos Martinsos commented Dec 14, 2023

This PR makes Wasp AI a proper, polished, well-documented feature of wasp CLI.

Resolves #1575 .

High-level list of changes:

  1. If you choose AI in wasp new, you get asked a set of interactive questions about which GPT models and which temperature you want to use (besides app name and description).
  2. wasp new:ai commands allows programmatically running Wasp AI. Its interface changed a bit, and it is now documented in the CLI usage output.
  3. Output of Wasp AI from wasp CLI when writing to disk is now enriched with newlines, colors, and some extra information. In the meantime, output towards Mage hasn't changed (due to some cool logic we implemented in Haskell).
  4. wasp-ai/ dir got renamed to mage/.
  5. Added README.md to mage/.
  6. Updated all READMEs (/README.md, /waspc/README.md) with relevant info about mage/. Also updated docs.
  7. Froze wasp-ai branch, from now on Mage will be deployed via release branch same as docs are deployed. Documented this in waspc/README.md.
  8. Updated Mage to work with these newest changes.

Some screenshots of using Wasp AI via wasp new, interactively:

ai1
ai2
ai3

@Martinsos Martinsos requested a review from infomiho December 14, 2023 13:23
mage/README.md Outdated Show resolved Hide resolved
liftIO $ createNewProjectOnDisk openAIApiKey waspProjectDir appName appDescription emptyNewProjectConfig
(planningGptModel, codingGptModel) <-
liftIO $
Interactive.askToChoose'
Copy link
Member Author

@Martinsos Martinsos Dec 14, 2023

Choose a reason for hiding this comment

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

@infomiho What I needed in this file was to first choose 2 GPT Models, then choose a number.
With how Interactive module currently works, I would need to create new types and make them instances of Option typeclass. That would be quite a bit of boilerplate, so I instead renamed typeclass Option to IsOption and added a new type to Interactive, called Option, that is a simple way to define an option. So now user of Interactive module has a choice -> they can use Option type or they can use some other type that is instance of IsOption.
Why I kept IsOption typeclass? Because it enables passing just Strings which is cool, and I imagine it can be convenient for some use cases.

Copy link
Contributor

@infomiho infomiho Dec 19, 2023

Choose a reason for hiding this comment

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

Makes sense 👍

One weird thing for me is the use of askToChoose' vs. askToChoose as the exposed function. Can we change that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, but both are valid choices to use -> one is for when dealing with Option, another is for more general case.

WHy not use askToChoose for all of them? Beacuse it would return Option ... and when you are using Option, you really want to get the value, not the whole option.

Since they are so similar, I thought ' works fine ok, it is a specialized version of askToChoose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, makes sense 👍


writeToLog "Generating project skeleton..."
let showParams chatGPTParams =
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I added some extra info: I show GPT model and temeprature

import qualified Data.Text as T
import qualified Wasp.Util.Terminal as Term

data LogMsg = Plain String | Concat [LogMsg] | Styled Style LogMsg
Copy link
Member Author

Choose a reason for hiding this comment

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

This is our "Decorated String" type.

<> "\n==================================\n\n\n\n"
)
$ return ()
debugTrace
Copy link
Member Author

Choose a reason for hiding this comment

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

We had this printing all the time before. Now, I made it so that it prints only when DEBUG env vars is set.

@@ -0,0 +1,23 @@
{-# OPTIONS_GHC -Wno-deprecations #-}
Copy link
Member Author

Choose a reason for hiding this comment

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

OK I went a bit wild here, I added this "fake" pure function that checks if we are in debug mode.

I spent some time investigating other ways to accomplish running code conditionally depending on DEBUG env var, which included stuff like cabal flags and compiler pragmas, but all the solutions ended up boing overly complex, and this just seems in practice and at the moment to be the easiest. We can revisit it in the future but I would waste time trying to find something "cleaner". I did read online that doing unsafe lookup of env var like this shouldn't be able to cause any problems, so we should be ok.


-- | Given a string, returns decorated string that when printed in terminal
-- will have same content as original string but will also exibit specified styles.
applyStyles :: [Style] -> String -> String
applyStyles [] str = str
applyStyles _ "" = ""
applyStyles styles str = foldl applyStyle str styles ++ escapeCode ++ resetCode
applyStyles styles str = foldl' applyStyle str styles ++ escapeCode ++ resetCode
Copy link
Member Author

Choose a reason for hiding this comment

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

Recent lesson about folds educated me enough to fix this :D.

showOption = id
showOptionDescription = const Nothing

data Option o = Option
Copy link
Member Author

Choose a reason for hiding this comment

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

To learn about the motivation for this change, check comment https://github.com/wasp-lang/wasp/pull/1601/files#r1426920624 .

Just description -> "\n" <> replicate indentLength ' ' <> description
Nothing -> ""
where
indentLength = 6
Copy link
Contributor

Choose a reason for hiding this comment

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

I preferred the look of the description being aligned below the title 🤷

Now it's a bit to the right
Screenshot 2023-12-19 at 12 40 28

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that was a mistake! I found that hard to read, it is harder to discern titles from descriptions. If you want it back I can revert it, but I like this better, think it is more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, it's obviously subjective, so I want insist on this 👍

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 maybe look at other CLI tools how they indent their titles and descriptions.

I found this in Astro's CLI
Screenshot 2023-12-21 at 15 18 14

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a reminder, this is what it looks like with old indent:
image

This does look harder to read to me, because all text is so crammed up. In the example above from Astro, they have newlines, so that solves the issue, detaches title from the text. We don't have that here, nor would it be a good fit.

That said, since it is not obvious that change I made is better (both to you and @sodic ), then I will revert it to previous state.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Fixing -> [Term.Green]
Error -> [Term.Red]
Custom styles -> styles
toTermString (Concat ms) = concat $ toTermString <$> ms
Copy link
Contributor

Choose a reason for hiding this comment

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

map would make this more readable on first read 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I find this more readable! Experience. But I suggest trying to get into it, I think once you get used to it, code gets easier to read. <$> is very common in Haskell, and you should get used to being able to read it quickly.

One thing I want to emphasize is that in Haskell, I most often don't bother understanding what every symbol does. I know compiler has my back and that stuff is "connected" correctly. What I care about is the general flow of data and what is happening.

So for example, when reading this code, I read it like this:

ms is taken out of Concat, then toTermString is applied to it, and then concat is applied to that.

Notice that I don't actually care too much, at least in the first read, if it is <$> or $, I see it all as application. It doesn't really matter to me, I know that the thing on the right is now "term stringified" and that at the end all stuff is concatenated.

And when reading code like that, it is easier to read it when using $ and <$> then when using fmap or map because they break my reading flow and force me to think about them.

Same is if I encounter an expression with a bunch of >>=, <$>, $, >>, &, ... .
Most of the time, I just need a general idea of what it does, and for that I don't need to understand exactly why each of those operators was used in that specific place. If I need to modify it, then I will dig a bit deeper, and again compiler will be helping me out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that map is more easier to read for developers that have < 2 years of Haskell experience. But as we talked about it IRL, this seems a matter of personal preference and as I said already, I won't press this matter any more.

@Martinsos
Copy link
Member Author

@infomiho I responded to all comments and fixed stuff, pls give it another look!

@infomiho
Copy link
Contributor

infomiho commented Dec 21, 2023

Looking good 👍

The indent change is a bit subjective, I prefer the old style 🤷

The fmap vs map is a bit subjective as well, so I won't press on that matter too hard.

@Martinsos
Copy link
Member Author

Looking good 👍

The indent change is a bit subjective, I prefer the old style 🤷

The fmap vs map is a bit subjective as well, so I won't press on that matter too hard.

Thanks! I reverted the indent change, and we agreed we will let people use what they want for map/fmap/<$>, so this should be it! If anything else let me know, otherwise I will merge once you approve.

Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

🚢 it!

@Martinsos Martinsos merged commit 994a58f into main Dec 22, 2023
4 checks passed
@Martinsos Martinsos deleted the wasp-ai-cli-mage branch December 22, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge wasp-ai into main and improve Mage's CLI DX
2 participants