[proposal] make the CLI available without cloning the repo #373

Eraz1997 · 2024-02-02T18:55:14Z

hey 👋

This repo and the job you did is amazing! I wanted to contribute with this PR with something I'd value when it comes to some of my personal projects. I'm not sure this is something you want to upstream as well, so I marked it as a "proposal".

With all these changes, I actually just moved the bin/train/main.py script inside the package to make it available to users installing the nam PyPi package without having to clone the whole repo.

Since there's no "bin" inside the bin folder anymore, I moved the bin/train one at the root level.

Finally, I aligned the README with these changes (+ I removed some old, I guess, paragraphs)

I hope it can be of any help 💪

Eraz1997 · 2024-02-02T19:28:10Z

Additional note, I’m honestly unsure whether running the CLI script outside of conda changes anything, even though from my understanding it shouldn’t

sdatkinson · 2024-02-07T16:33:04Z

I like this idea. However, your PR does unfortunately introduce a lot of issues that need to get addressed before I can merge it.

I'll need to find some time to review this.

Eraz1997 · 2024-02-08T12:26:15Z

Oh, I’m sorry I haven’t noticed them! If you give me some pointers I can try to address the issues 💪

One thing I just noticed is that pytorch-cuda is missing if you don’t run the CLI inside the conda environment. I can think about a solution, if needed.

sdatkinson

See notes

nam/train/cli.py

sdatkinson · 2024-02-07T16:28:47Z

README.md

@@ -29,32 +29,9 @@ After installing the Python package, a GUI can be accessed by running `nam` in t

 ### The command line trainer (all features)

-Alternatively, you can clone this repo to your computer and use it locally.


[BLOCKING] Don't delete this. This is instructions for how to do development with the repo.

It's ok w/ me if you want to add some small section saying that pip install neural-amp-modeler is a thing if you want to depend on it w/o modifying it. But I want developers to understand how to correctly set up their development environment.

Sounds good 👌 My (obviously questionable) suggestion is to clarify what is a CONTRIBUTING guide and what are usage guidelines. This paragraph looks more like user guidelines, so I'd specify that.

I'll leave them as they are and possibly add just a paragraph about pip 💪

README.md

nam/_version.py

.gitignore

tests/test_nam/test_train/test_cli.py

sdatkinson · 2024-03-10T18:10:00Z

nam/train/cli.py

@@ -225,11 +225,15 @@ def main_inner(
    model.net.export(outdir)


-if __name__ == "__main__":
+def main():


[Nit] Now that there are 3 different "entry points" with different interfaces, it'd probably be good to docstring why they're all here and what they achieve [I can handle this.]

train/easy_colab.ipynb

Eraz1997 · 2024-03-26T17:51:31Z

I rewrote the PR from scratch as I thought it'd have been simpler - and indeed I came up with way fewer and leaner changes IMHO 🚀

Sorry for the first round of review, it must have been tough as the PR was a bit messy and changed a lot of things I didn't quite check you agreed with before 🙏

What mainly changes from the "previous version of the PR" is:

I kept files at their place
I didn't remove any documentation, just added a tiny paragraph about pip and the new nam-cli command

I kept files at their place

I guess this is what you were referring to when you mentioned the broken test - indeed nam-cli was working perfectly fine, but if you meant the python bin/train/main.py ... command, then yes, it broke because I intentionally replaced it with nam-cli, but indeed I should have checked it with you first! 🙏

Thanks!! 😊

sdatkinson

Overall, this looks great!

I can take care of the nits and checks myself, but thanks for taking the initiative on this--I like it 👍🏻

sdatkinson · 2024-07-23T01:44:10Z

docs/source/tutorials/command-line.rst

@@ -38,6 +38,29 @@ Then activate the environment you've created with

    conda activate nam

+Alternative: directly install the PyPi package


sdatkinson · 2024-07-23T01:45:28Z

docs/source/tutorials/command-line.rst

+
+   pip install neural-amp-modeler
+
+In this case, you can just run `nam-cli` in your shell instead of cloning the repository and running the `bin/train/main.py` script as explained below in the training section.


In rst it's two backticks, not one (little different from Markdown)

Let's call it nam-full

sdatkinson · 2024-07-23T01:46:21Z

bin/train/main.py

-        plot(model, dataset_validation, show=not no_show)
-    # Export!
-    model.net.export(outdir)
+from nam.train.cli import run


Know what? Let's just remove this file altogether. I'm looking to do a release that makes breaking changes; might as well put this break as well.

Sounds great to me 🚀

sdatkinson · 2024-07-23T01:47:39Z

nam/train/cli.py

+# See:
+# https://github.com/sdatkinson/neural-amp-modeler/issues/105
+# https://stackoverflow.com/a/44822794
+def _ensure_graceful_shutdowns():


[Critical] Does this still work? I'll want to check.

sdatkinson · 2024-07-23T01:48:30Z

setup.py

@@ -55,6 +55,7 @@ def get_additional_requirements():
    entry_points={
        "console_scripts": [
            "nam = nam.train.gui:run",
+            "nam-cli = nam.train.cli:run",


Eraz1997 · 2024-07-23T06:17:56Z

Great!! LMK if you want me to help somehow 🚀 Thank you!

sdatkinson · 2024-07-23T15:25:06Z

@Eraz1997 on second thought, if you can grab those changes before Thursday you'll probably get to it before me :)

Eraz1997 · 2024-07-23T15:40:45Z

@sdatkinson unfortunately I don't have band this week, LMK if it can be of any help next week as well!

sdatkinson · 2024-07-27T20:02:03Z

Got the nits and did a bit of cleanup on the documentation in Eraz1997/main.

Closing this PR and we'll merge there 👍🏻

Eraz1997 · 2024-07-29T08:28:23Z

Perfect!! 🚀

feat: make the CLI available without cloning the repo

c04ee52

Eraz1997 added 2 commits February 3, 2024 19:41

fix: remove accidental output folder

f5c185b

chore: bump version

cc84072

sdatkinson requested changes Mar 10, 2024

View reviewed changes

Eraz1997 force-pushed the main branch from fe54082 to c3b2ac2 Compare March 26, 2024 17:03

fix: revert standalone CLI with pip

e41b595

Eraz1997 force-pushed the main branch from c3b2ac2 to e41b595 Compare March 26, 2024 17:07

Eraz1997 and others added 2 commits March 26, 2024 18:07

Merge branch 'sdatkinson:main' into main

3bcfea8

feat: add cli command to “console_scripts”

e2893e5

Eraz1997 requested a review from sdatkinson March 26, 2024 17:53

sdatkinson reviewed Jul 23, 2024

View reviewed changes

sdatkinson closed this Jul 27, 2024

sdatkinson mentioned this pull request Jul 27, 2024

[FEATURE,BREAKING] Define full-featured trainer as a CLI entry point #445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proposal] make the CLI available without cloning the repo #373

[proposal] make the CLI available without cloning the repo #373

Eraz1997 commented Feb 2, 2024

Eraz1997 commented Feb 2, 2024 •

edited

Loading

sdatkinson commented Feb 7, 2024

Eraz1997 commented Feb 8, 2024

sdatkinson left a comment

sdatkinson Feb 7, 2024

Eraz1997 Mar 26, 2024 •

edited

Loading

sdatkinson Mar 10, 2024

Eraz1997 commented Mar 26, 2024

sdatkinson left a comment

sdatkinson Jul 23, 2024

sdatkinson Jul 23, 2024

sdatkinson Jul 23, 2024

Eraz1997 Jul 23, 2024

sdatkinson Jul 23, 2024

sdatkinson Jul 23, 2024

Eraz1997 commented Jul 23, 2024

sdatkinson commented Jul 23, 2024

Eraz1997 commented Jul 23, 2024

sdatkinson commented Jul 27, 2024

Eraz1997 commented Jul 29, 2024

		@@ -29,32 +29,9 @@ After installing the Python package, a GUI can be accessed by running `nam` in t

		### The command line trainer (all features)

		Alternatively, you can clone this repo to your computer and use it locally.

		@@ -38,6 +38,29 @@ Then activate the environment you've created with

		conda activate nam

		Alternative: directly install the PyPi package


		pip install neural-amp-modeler

		In this case, you can just run `nam-cli` in your shell instead of cloning the repository and running the `bin/train/main.py` script as explained below in the training section.

[proposal] make the CLI available without cloning the repo #373

[proposal] make the CLI available without cloning the repo #373

Conversation

Eraz1997 commented Feb 2, 2024

Eraz1997 commented Feb 2, 2024 • edited Loading

sdatkinson commented Feb 7, 2024

Eraz1997 commented Feb 8, 2024

sdatkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eraz1997 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eraz1997 commented Mar 26, 2024

sdatkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eraz1997 commented Jul 23, 2024

sdatkinson commented Jul 23, 2024

Eraz1997 commented Jul 23, 2024

sdatkinson commented Jul 27, 2024

Eraz1997 commented Jul 29, 2024

Eraz1997 commented Feb 2, 2024 •

edited

Loading

Eraz1997 Mar 26, 2024 •

edited

Loading