Remove duplicate flag --repo_path from the fetch command (#1370)

[snufkinwa]

Thank you for taking your time to contribute to Ersilia, just a few checks before we proceed

• [x] Have you followed the guidelines in our Contribution Guide
• [x] Have you written new tests for your core changes, as applicable?
• [x] Have you successfully ran tests with your changes locally?

Description

The --repo_path and --from_dir flags both serve the same purpose: specifying the local path to a model's directory. Right now, --from_dir is only used to set the model source, while most users rely on --repo_path to load local models. To keep naming consistent with other flags (like --from_github and --from_dockerhub), we should use --from_dir and remove --repo_path.

Changes to be made

• [x] Remove the --repo_path flag in favor of only keeping the --from_dir flag.
• [x] For sake of backward compatibility and not breaking the rest of the code, we can set repo_path argument in the ModelFetcher class's constructor with the value the user inputs through from_dir.

Status

Ready to Review

To do

• [x] Implementing test for changes, making sure the fetch command is working properly

  
  import unittest
  from ersilia.hub.fetch.fetch import ModelFetcher

class TestModelFetcher(unittest.TestCase):

def test_from_dir_priority_when_repo_path_is_set(self):
    fetcher = ModelFetcher(from_dir="test_repo_path")
    self.assertEqual(fetcher.repo_path, "test_repo_path", "repo_path should match the value provided to from_dir")

def test_from_dir_fallback_when_repo_path_is_none(self):
    fetcher = ModelFetcher(from_dir="fallback_dir")
    self.assertEqual(fetcher.repo_path, "fallback_dir", "from_dir should be used as the primary path")

def test_no_repo_path_or_from_dir(self):
    fetcher = ModelFetcher(from_dir=None)
    self.assertIsNone(fetcher.repo_path, "repo_path should be None if from_dir is None")

if name == 'main': unittest.main()

Is this pull request related to any open issue? If yes, replace issueID below with the issue ID

Related to #1370 

[snufkinwa]

Removing @click.option("--repo_path", "-r", default=None, type=click.STRING) causes the code to break. To fix this, we can use the or operator in the ModelFetcher constructor to prioritize from_dir as the replacement for repo_path.

On my Mac, CMake is missing, so while the fetch command works to retrieve the model, the build process fails. I ran test_models.py, that successfully completed without issues. Additionally, I wrote a few unit tests to ensure --from_dir is prioritized correctly in the absence of repo_path.

[DhanshreeA]

@snufkinwa we actually need to remove the flag from the CLI which I don't see being addressed in this PR.

[snufkinwa]

Everything on /hub/fetch/fetch.py was reverted back, that was the reason I was getting error when trying to remove --repo_path, then add from_dir to the constructor. The only file edited was cli/commands/fetch.py. Tested several times with test_models.py It fetched successfully.

[snufkinwa]

  File "/home/runner/work/ersilia/ersilia/ersilia/cli/commands/__init__.py", line 22, in wrapper
    return func(*args, **kwargs)
TypeError: fetch_cmd.<locals>.fetch() missing 1 required positional argument: 'repo_path'

This is the error I was getting originally, this happens with the removal of @click.option("--repo_path", "-r", default=None, type=click.STRING)

The test_models.py was tested several times, it didn't give me that error when fetching, but the additional test on the PR produced the same error. It has something to do with BentoML, as the line in question is in function def bentoml_common_params(func)

[DhanshreeA]

  File "/home/runner/work/ersilia/ersilia/ersilia/cli/commands/__init__.py", line 22, in wrapper
    return func(*args, **kwargs)
TypeError: fetch_cmd.<locals>.fetch() missing 1 required positional argument: 'repo_path'

This is the error I was getting originally, this happens with the removal of @click.option("--repo_path", "-r", default=None, type=click.STRING)

The test_models.py was tested several times, it didn't give me that error when fetching, but the additional test on the PR produced the same error. It has something to do with BentoML, as the line in question is in function def bentoml_common_params(func)

@snufkinwa this happens because we still need to remove repo_path from the arguments that the fetch function gets here.

[DhanshreeA]

LGTM @snufkinwa thanks!