[Refactoring] Use Typer instead of argparse?

[paketb0te]

Hi @initialcommit-io , it's me again 😆

With the inheritance model simplified, I think there is one other thing we could greatly improve - the way the command line arguments are handled.

I am a big fan of Typer, that tool makes it super easy to manage all that stuff - and it gives type hints!

The docs are also great and explain the whole concept better than I could ever do here, so maybe you want to have a look at that :)

I have worked on a branch showcasing the concept for some of the commands (log and status for now), check it out: typer

The basic idea is that each command is a function, and the function arguments are the corresponding CLI arguments / flags / options. But we don't have to do all that argparse stuff (setting up parsers and subparsers in main(), and having to sync that with the args.subcommand in the actual Command classes ...).

Let me know if you like it!

[initialcommit-io]

Hi again @paketb0te!

Wow cool! At a quick glance, I do like it. I like how it keeps just the global options in the __main__.py and the subcommand options in the individual subcommand modules. Seems easier for the workflow to have all the stuff for each subcommand in one place.

I also like that we get rid of that big if statement.

I need to take a deeper look into some of the functionality to fully understand how it works. I'll keep you posted on that but I like the direction this is going in.

Here are a few thoughts/questions:

• So for the Settings, how should we decide what should be included there? We use default values both in the global options, subcommand options that are unique to each subcommand and also in some subcommand specific __init__() methods. Would it just be anything in those categories that has a default value?

• One thing I want to make sure we can support is sub-sub commands. By that, I mean stuff like $ git stash pop and $ git stash push <filename>. I hadn't looked into that with argparse yet but it was on my agenda. It would be great to know there is a clean way to do this with Typer before switching.

• This is a small thing, but in animations.py we'd need to be able to get the command name to include in the output image/video file here: image_file_name = "git-sim-log_" + t + ".jpg"

• Also I think I like changing the git_sim_subcommand.py filenames to just subcommand.py, like log.py and status.py (as well as the corresponding class names), just seems cleaner that way. But we can keep git_sim_base_command.py as is.

[paketb0te]

So for the Settings, how should we decide what should be included there?

I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints).

One thing I want to make sure we can support is sub-sub commands.

That is possible (and actually super easy), I have tried it already (see the subcommands section in the typer docs). You basically just create a new typer app for the subcommand and add it to the main app.

in animations.py we'd need to be able to get the command name

I think that should be fairly straightforward. Even though there does not seem to be an easy way for a function to know it's own name, we could always just add this to the Settings class or pass it explicitly to handle_animations()

I like changing the git_sim_subcommand.py filenames

Main reason was that I wanted to look at the original files while creating the examples, but if you like it yeah we can of course rename them. Maybe we even move them to a commands directory or something? 🤔

re: renaming, I think it would make sense to rename the GitSimCommand classes to GitSimCommandScene or CommandScene (because they are the Scene objects for Manim :D )

What do you think?

[paketb0te]

@initialcommit-io unfortunately this change would have to be done for all commands at once (at least I am not aware of a way to have arparse and Typer work at the same time to have a gradual change).

I am more than willing to do the work, but would like to make sure we are on the same page about this so that my time is not wasted :)

[initialcommit-io]

@paketb0te Hey sorry for the delay!

I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints).

Ok cool - one thing to note is that some of the subcommand subclasses override properties from the GitSimBaseCommand. For example, the GitSimLog overrides self.numCommits and self.defaultNumCommits. Another example is GitSimRevert which overrides several other properties too.

Also, some subclasses might have their own new properties specific to that subcommand, that aren't implemented in the parent.

Maybe we even move them to a commands directory or something?

For now, lets keep them in the current directory until it gets too unruly 😄 . Since the command subclass modules are most of the files anyway, moving them to a subfolder wouldn't be that much cleaner for now, and would involve browsing into another subdir during development which is a bit annoying.

renaming, I think it would make sense to rename...

This makes sense, but I wonder if we should favor the cleaner approach of just naming the subcommand classes as Log, Status, Rebase, etc. I think in the context of the program it should be pretty easy to figure out they are scenes, and their main purpose is to identify the subcommand that is being represented.

unfortunately this change would have to be done for all commands at once

No worries! That's fine - and yes I do think we should move in this direction and implement this. We'll just need to make sure to test it thoroughly before releasing it. Appreciate all your time, input, and help with this I think it is a much cleaner and more intuitive way to organize this stuff.

[paketb0te]

@initialcommit-io regarding the subcommands' properties like numCommits etc, I am a bit on the fence if we should store them in a dataclass like Settings (and then reference them like Settings.commits everywhere, or if it is better to explicitly pass them as arguments to GitSimBaseCommand. I tend towards the first, do you have any preference?

(Same goes for command-specific properties)

For now, lets keep them in the current directory

Yeah makes sense 😄

naming the subcommand classes as Log, Status, Rebase, etc

Agreed, it's probably obvious enough what those are doing.

Will start working on it, but will definitely take a bit of time :)

[paketb0te]

@initialcommit-io quick question: what does the self.maxrefs in GitSimStatus do?

class GitSimStatus(GitSimBaseCommand):
    def __init__(self, args: Namespace):
        super().__init__(args=args)
        self.maxrefs = 2
        self.hide_first_tag = True
        self.allow_no_commits = True

When searching the project, I see that variable set for a few of the subcommands, but never actually accessed... is that something internal to Manim? Do we need it?

[initialcommit-io]

@paketb0te I think I used that previously to set the max number of branch/tag labels that would show on top of a single commit, but I think at some point I switched to using args.max_branches_per_commit and args.max_tags_per_commit and forgot to remove the maxrefs. So yes I think we can remove that. Good catch!

[paketb0te]

Made some progress today (long train ride), only revert, rebase, merge, cherrypick are left to change, as well as some cleaning up.

@initialcommit-io FYI, I will be away for a few weeks starting february 17th, so I try to finish everything this week, hopefully you can have a look at it over the weekend so we can get this done before I leave 🤞

[initialcommit-io]

@paketb0te Alright! I will do my best to take a look this weekend and merge the PR. Thanks again!

[initialcommit-io]

@paketb0te Just fixed a minor import bug, but aside from that seems to be working well! I haven't thoroughly tested but the basic stuff seems to be working 😄 .

Any other suggestions for this one for now before we close it out?

[initialcommit-io]

@paketb0te Hmm question about the cherrypick command/class. In Git, the command is actually cherry-pick, (with the hyphen) which is what it was using argparse. After changing to Typer, it is now set as "cherrypick" with no hyphen. Can you take a look at that?

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/". Also just want to make sure the environment variable config that we implemented still works...

[initialcommit-io]

@paketb0te Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in.

[paketb0te]

question about the cherrypick command/class

Ah, I was not aware of that - we can just rename the function to cherry_pick(), underscores in function names are automatically mapped to hyphens in the commands/arguments/options names :)

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/".

I just moved the relevant code into animytions.py, I did not expect that behavior
to change 🤔

On that note, I think we can improve the readability of that part by using pathlib, which can abstract the underlying OS path so we don't need to worry about forward-slash / back-slash handling manually. I'll have a look at it.

Also just want to make sure the environment variable config that we implemented still works...

There is this awesome tool pydantic which can do that for us :D

(we can just make the Settings inherit from pydantic's BaseSettings class, then we get the whole "reading env vars into variables" for free - see Settings management)

[paketb0te]

Re: the pydantic Settings, see #52 :)

[paketb0te]

Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in.

I'll have a look at that.

[paketb0te]

@initialcommit-io I only see the -e shorthand for git-sim cherry-pick, not for rebase... a typo, or did I miss something?

[initialcommit-io]

@paketb0te Sounds good, merged the 2 PR's. Yes you're right for -e shorthand I meant to say cherrypick, not rebase.

A few notes:
-It looks like the shorthand args still need to be reimplemented
-For the pydantic env variables, can you provide an example of how to set one or two of them, esp the media dir? That way I can test and add to the README
-The program is still creating a new media/ directory in the location that git-sim is run

[paketb0te]

-It looks like the shorthand args still need to be reimplemented

-> #53

For the pydantic env variables, can you provide an example of how to set one or two of them

It looks like I broke something there, it is not working as it did when playing with a minimal demo 🤔

I'l look into it

[initialcommit-io]

@paketb0te Oh really? What command broke for you? Things seem to be working ok for me. Main bug that I see is it's still creating the media/ directory...

[paketb0te]

@initialcommit-io eh, I just had a typo, therefore my environment variables did not get picked up 😅

-> #54

Regarding the media dir issue, I have to check that out over the weekend.

[initialcommit-io]

Ok great!!! Merged. Thanks for everything 😃

[paketb0te]

For the pydantic env variables, can you provide an example of how to set one or two of them

# Export environment variable for multiple uses
export GIT_SIM_ANIMATE=true
git-sim log
git-sim commit

# Use environment variable for a single command
GIT_SIM_ANIMATE=true git-sim log

I think pydantic can even read from .env files, but I am not 100% sure.

Edit: Yes, it does: Dotenv (.env) support

[initialcommit-io]

@paketb0te Awesome!! That's a really cool and convenient feature that it allows configuring any of the settings as env variables like that!

FYI I was playing with the media_dir issue and it's kinda weird. It seems the manim config options are not being set for some reason, such as config.media_dir, config.background_color, etc.

So it's not just the media_dir, it's anything that gets set like config.xyz. You can confirm this by trying to run git-sim --light-mode log, and notice the background color stays black.

It's acting like something in the new setup is causing the config options to be set on a different instance of the manim config object, i.e. not the global one for some reason. Here is a link to the manim config docs: https://docs.manim.community/en/stable/guides/configuration.html#the-manimconfig-class

Can you take a look at that to try and make sure the global config class is the one we are setting the properties on? I think that should fix these issues...

---

One other thing - when @abhijitnathwani added the original environment variable git_sim_media_dir, he appended the repo_name to the supplied path (the logic to build the repo name string is still in there, but no longer used) to compartmentalize the stuff for different repos, so it's like: git_sim_media_dir + "git-sim_media" + repo_name

That should only happen if the environment variable value is set and the command line --media_dir option is default, so I think we might need to add back some of @abhijitnathwani 's code for that, it was this:

# If the env variable is set and no argument provided, use the env variable value
-    if os.getenv("git_sim_media_dir") and Settings.media_dir == ".":
-        config.media_dir = os.path.join(
-            os.path.expanduser(os.getenv("git_sim_media_dir")),
-            "git-sim_media",
-            repo_name,
-        )

[abhijitnathwani]

Hi @paketb0te @initialcommit-io ,
These are some BIG changes in the whole flow of the application. I need to re-map the application flow and understand how to use Typer. It looks intuitive, however, need to go through the docs to understand the usage.

FYI, these changes don't work on certain python 3 versions. I tried pulling the latest changes and it stopped working. I tested with python 3.8.6 and python 3.9.0. I was met with the following error:

    File "D:\git-sim\git_sim\settings.py", line 22, in Settings
    files: list[pathlib.Path] | None = None
TypeError: 'type' object is not subscriptable

I upgraded to python 3.11.2, the latest available, and it started working. These changes may be breaking for some users as they may not always use the latest versions. I suggest we make it compatible with at least upto Python 3.8. If not, we need to make changes to README, dockerfile and some graceful message asking user to update their python version to run git-sim, which imho, doesn't make sense.

[initialcommit-io]

@abhijitnathwani Yes the refactoring with Typer and Pydantic does restructure the code nicely, although we are still hammering out some kinks as you can tell.

Great catch on the Python 3 compatibility issues - I have been testing in 3.11.1 so would have missed those breaking changes for the earlier versions (which also speaks to needing a good test plan like we've been discussing and now have the other thread for). I will wait to push a new release to PyPI until we have everything sorted out and validated in various Python versions.

I agree with the need to support the earlier Python versions, upon release the tool supported all the way back to 3.7, so that would be ideal.

[initialcommit-io]

@paketb0te I was working on this and moving the Manim config stuff back into the main() function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon.

[paketb0te]

@paketb0te I was working on this and moving the Manim config stuff back into the main() function is the only place where it seems to get set properly. I'm not sure exactly why that happens, but I am going to refactor it back over there so we can keep moving forward. I will make that commit soon.

Hm, when inspecting the Manim config, I actually see that the values get passed in correctly into manims's config object, but when I look at scene.renderer.file_writer.movie_file_path, it uses the media dir, instead of git-sim_media 🤔

I think it should not make a difference whether the animation logic is called from main() or from anywhere else, so I am trying to understand what the actual problem is we are facing, and what is just a symptom resulting from it...

[initialcommit-io]

@paketb0te I moved the config stuff back into main along with a few minor updates to get it all working again. It's only about 20 lines of code so it doesn't clutter up main() too much. From my testing it seemed like there were multiple instances of the manim config or something, so setting the config values in another module seemed to not be affecting the correct instance. Still not totally sure tho, but it works fine after moving into the main() method.

[initialcommit-io]

@paketb0te @abhijitnathwani I fixed the compatibility issues with older versions of Python (from 3.7 and up), refactored the manim config stuff as mentioned above, and made various other updates to release v0.2.3.

So I think our Typer setup is here pretty much completed for now. Closing this for now as the first version with Typer has been implemented and released.

Let's move various other/related conversations (like test suite) into "Discussions" or a relevant issue.