Skip to content

Implementation of the find repo functionality #56

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

Merged
merged 4 commits into from
Feb 8, 2025

Conversation

Sid9993
Copy link
Contributor

@Sid9993 Sid9993 commented Feb 6, 2025

Implemented the find repo cmd functionality to get the repo path location. The allowed formats for using find repo are :

  • <repo_owner>@<repos_name>
  • <repo_url>
  • <repo_uid>
  • <repo_alias>
  • <repo_alias>,<repo_uid>

@Sid9993 Sid9993 requested a review from a team as a code owner February 6, 2025 20:52
Copy link

github-actions bot commented Feb 6, 2025

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

try:
# Get repos_list using the existing method
repos_list = self.load_repos_and_meta()
if(run_args.get('item', run_args.get('artifact'))):
Copy link
Contributor

@anandhu-eng anandhu-eng Feb 8, 2025

Choose a reason for hiding this comment

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

Do we need an if condition here since repo, item, and artifact are being considered under else?

lst.append(i)
try:
# Get repos_list using the existing method
repos_list = self.load_repos_and_meta()
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't the load_repos_and_meta be automatically called when initialising the Action class while running mlc? I think it could be accessed through self.repos_list .

lst.append(i)
elif repo_name == i.meta['alias']:
lst.append(i)
elif self.is_uid(repo) and not any(i.meta['uid'] == repo_uid for i in self.repos):
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this looping create un-necessary complexity as we are already inside a loop which tries to do the same function in more generic way?

lst.append(i)
elif self.is_uid(repo) and not any(i.meta['uid'] == repo_uid for i in self.repos):
return {"return": 1, "error": f"No repository with UID: '{repo_uid}' was found"}
elif "," in repo and not matched_repo_path and not any(i.meta['uid'] == repo_uid for i in self.repos) and not any(i.meta['alias'] == repo_name for i in self.repos):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I think usage of any could reduce the number of lines but with a different logic.

@Sid9993 do you think the code would be more efficient and small if we use the below snippet with revised logic?

matching_repos = [i for i in self.repos if i.meta['uid'] == repo_uid]

@@ -1462,14 +1514,14 @@ def mlcr():

def process_console_output(res, target, action, run_args):
if action == "find":
if "list" not in res:
return # Exit function if there's an error
Copy link
Contributor

Choose a reason for hiding this comment

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

The check for key list is nice to have, while returning, we usually follow the pattern below for error:

return {"return": error-code, "error": detail-of-error}

@arjunsuresh arjunsuresh merged commit 8d1dc6e into mlcommons:dev Feb 8, 2025
29 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants