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

Use rich for displaying error traceback in mlinfra cli #129

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

ara-25
Copy link
Contributor

@ara-25 ara-25 commented Oct 23, 2024

This PR proposes the following changes

Closes #96

PR Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Message that will be displayed on users' first pull request

@aliabbasjaffri
Copy link
Collaborator

@ara-25 have you tested these changes? I tried to see rich based stacktrace, but that didn't work.

@ara-25
Copy link
Contributor Author

ara-25 commented Oct 24, 2024

@ara-25 have you tested these changes? I tried to see rich based stacktrace, but that didn't work.

That's strange. Are you testing by running the mlinfra cli or some other module?

For me, the stack trace is showing upon running mlinfra estimate-cost abc.yml

@aliabbasjaffri
Copy link
Collaborator

I'm testing it by trying to run the pyinvoke command directly, so by calling pyinvoke estimate-cost abs.yaml.

@ara-25
Copy link
Contributor Author

ara-25 commented Oct 25, 2024

So, with my current implementation, the rich traceback only appears in mlinfra command or if cli.py is run directly. This is because from the original issue, I understood that this feature only needs to be added for the mlinfra cli, instead of the entire codebase.

Please let me know if the formatted traceback needs to be displayed for the entire codebase and I will modify it accordingly.

@aliabbasjaffri
Copy link
Collaborator

Ah alright, that makes sense.
Yes, the idea was to have this enabled for the entire codebase as it would be easier to spot where an error is coming from. It would be really cool if you can adjust it for the repo, or if you want i can create a new issue for that. Both are fine with me.

@ara-25
Copy link
Contributor Author

ara-25 commented Oct 26, 2024

@aliabbasjaffri then would be great if I am able to take it up as part of a separate issue :)

@aliabbasjaffri
Copy link
Collaborator

Just tested and verified. Thank you for your contribution. If you are interested in contributing further and extending rich to the rest of the codebase, do let me know.

@aliabbasjaffri aliabbasjaffri merged commit 25f3e98 into mlinfra-io:main Oct 28, 2024
5 checks passed
@aliabbasjaffri aliabbasjaffri added the hacktoberfest-accepted Label for accepted PRs for Hacktoberfest label Oct 28, 2024
@ara-25
Copy link
Contributor Author

ara-25 commented Oct 29, 2024

Thanks @aliabbasjaffri

I am interested on working further on integrating rich in mlinfra. Feel free to assign any future tasks

@aliabbasjaffri
Copy link
Collaborator

hey @ara-25 , i managed to create a new issue for rich integration across entire codebase. Please let me know if you need any more context of help regarding this. You can reach out to me via discord channel, i'm rather fast to reply back there than on the github issues.

Also, i would like to request a github ⭐ from you as that would really help the tool get more traction and users. Thank you!

@ara-25
Copy link
Contributor Author

ara-25 commented Oct 30, 2024

Thanks a lot @aliabbasjaffri.

Also, I have already starred your amazing work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Label for accepted PRs for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add rich to mlinfra cli to allow better understanding of stacktrace
2 participants