Adding Red Queen CLI#27
Conversation
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
I only took a look at the setup.py so far to help with the packaging issues.I left some inline comments on how I assume you'll be able to fix the issues @raynelfss is hitting when testing this locally.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
danielleodigie
left a comment
danielleodigie
left a comment
There was a problem hiding this comment.
Looks good, just a couple small changes and questions!
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
Hello Caleb. Your CLI works like a charm. But Windows is still having issues, although this one change seems to solve it.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
This is progressing nicely. I haven't had a chance to go through all the logic in detail yet. But I have some high level comments inline from a quick scan of the code.
|
|
||
| <br> | ||
|
|
||
| <h1>Usage</h1> |
There was a problem hiding this comment.
Does using explicit html markup for headers and breaks improve the formatting here? I think we should stick to the markdown formatting if not.
There was a problem hiding this comment.
I was looking into it, and it doesn't really add much. The Web Dev in me wanted it to make sense, but it doesn't. I'll adjust it accordingly.
| # Part of Red Queen Project. This file is distributed under the Apache 2.0. | ||
| # See accompanying file /LICENSE for details. | ||
| # ------------------------------------------------------------------------------ | ||
| """Shebang is used to make this code an python executable""" |
There was a problem hiding this comment.
You don't really need to say this as it's a standard convention on unix systems for interpreted programs.
Instead you should have a module docstring that describes the purpose of the python code in this file
There was a problem hiding this comment.
Okay, I just pushed a docstring that is more descriptive.
| def show_result(): | ||
| results_dict = result_retrieval() | ||
| result_num = max(results_dict.keys()) | ||
| result_path = tuple(results_dict[result_num].values())[0] | ||
| command_list = ["python3", "-m", "report.console_tables", "--storage"] | ||
| command_list.append(str(result_path)) | ||
| subprocess.run( | ||
| command_list, | ||
| check=True, | ||
| ) | ||
| click.echo("To view the table again:") | ||
| click.echo(" ".join(command_list)) |
There was a problem hiding this comment.
Ideally we should be able to call this directly via python. It looks like I forgot to move the report directory into red_queen as part of #21 which precludes doing this now. So this is fine for now. But after this merges we should move the report stuff into the red queen package and then basically make this function something like:
from red_queen.report import table_visualizer
table_visualizer(result_path)
There was a problem hiding this comment.
If anything, once we get the CLI merged, open that as an issue. And then, I'll try my best to address it in a timely manor.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| # main() |
There was a problem hiding this comment.
Should this be commented? I assume if you call this file standalone you want it to run the CLI too:
| # main() | |
| main() |
There was a problem hiding this comment.
Good catch! I was testing out code and forgot comment back in main().
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sys.exacutable doesn't work for its intended application, because it gives the path of the red-queen python package instead of the python path. So I have replaced the line that had sys.exacutable with python. I did this because I want the result table of the benchmark exacutaion to be displayed.
… Usage In the commit, I have added documentation for how some sets up their machine to use Red Queen properly, and along with that how to ultize Red Queen with its CLI.
In this commit, I added .virtualenv to the .gitignore file, so that people who follow along with the documentation don't have to worry about their virtual enviornment affecting their commits
|
I have finished the documentation for the CLI, and feel that it is ready to merge. However, when the CI job run on my branch, it is saying the code is failing the lint test. This is odd because when I ran |
In this commit, I have added the __init__.py for the red_queen/games/applications/benchmarks/ directory. I am hoping that this allow fixes the merge issue for my pull request.
In this commit, I have removed an extra init file as it is not a part of the main Red Queen Repository, and it has nothing to do with the CLI.
In this commit I have done a couple of things: One, I have simplified the logic for the benchmark handler. Before if someone wanted to run a benchmark they would have to specify the benchmark type first. I forsaw this causing issues, so I made it so that all a user would have to do is supply the cli with the benchmark file name. Two, I reduced the benchmark and complier retrival function into one since they both do the same thing, so it just reduces clutter in code. Three, I changed the name of variables in the hopes of making it easier to understand what each function call is doing All in all, I believe that this was a neccessary first step in making this cli a reliable tool that anyone in the quantum computing sapce can ultize
I just modified the formatting of thr cli, so that it would pass tox.
In this commit, I added information about the cli and how to run virtual environments
4 participants
No description provided.