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

Replac --json with --outfile flag for query commands (result/status). #469

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

dmageeLANL
Copy link
Collaborator

It's more useful to send json output to a json file. and more useful in general to have an outfile option so that any type of output can be saved to a file.

This commit replaces the json flag with an outfile flag. If the user specifies a file that doesn't have a '.json' extension, pavilion will print the usual output to that file instead of stdout. If it does have a 'json' extension, it'll dump the relevant json data structure to that file. This way a single flag handles both use cases, the desired option to output the query results to a file, and the current option to format that output as a pavilion table or a json object.

…outfile flag, default is self.outfile, which, if set, will redirect output to the specified file. If that file ends in '.json', then json output of the specified type will be dumped to that file.
@Paul-Ferrell
Copy link
Collaborator

Paul-Ferrell commented Feb 15, 2022

I'm REALLY glad you made this a separate merge request because - I hate it.

  1. You're trying to do the job of the shell. --outfile is equivalent to > outfile.
  2. You're implicitly changing output formats. If I want json in a .fuck-you-i-will-do-what-i-want-file, I want to be able to redirect it there without caring about extension.
  3. It breaks the general unix idea of extensions don't matter - use filemagic (file command) to figure out file types.

There is a place for this - with binary output. That's why commands like dd take an outfile, and almost no other commands do.

@Paul-Ferrell
Copy link
Collaborator

I'm not closing this. I'll bring it up at the testing meeting tomorrow instead. I'm not the Pavilion dictator.

@dmageeLANL
Copy link
Collaborator Author

No other commands take an outfile? Like all the compilers, tar (compression):
curl:

curl -h | grep output
 -o, --output <file> Write to file instead of stdout

wget:
➜ ~ wget -h | grep out
-O, --output-document=FILE write documents to FILE

Even query commands like nvidia-smi:
``` bash
nvidia-smi -h | grep out
    -f,   --filename=           Log to a specified file, rather than to stdout.

I agree it's less common or a program to make inferences about arguments based on their attributes. But some do. Part of the inspiration for this is git archive (doc. Fifth paragraph down:

The proceeding example will create a new archive and store it in the exmaple_repo_archive.tar file. The previous examples have both created uncompressed archive output. This is denoted by the --format=tar option. The format option also accepts popular compressed file formats zip and tar.gz. Passing one of these format options will produce a compressed archive. If a format value is not passed it will be inferred from any --output option passed.

But in general, this just seemed like a simpler way to accomplish what I couldn't figure out how to accomplish with the -j flag. What I originally wanted was something like this:

pav results # OUTPUT TABLE TO STDOUT
pav results -j # OUTPUT JSON TO STDOUT
pav results -j outfile.json # OUTPUT JSON TO FILE

Because how can you guarantee that the only stdout is a valid json, it seems like there's a lot of places that can go wrong.

@Paul-Ferrell
Copy link
Collaborator

Paul-Ferrell commented Feb 16, 2022 via email

@dmageeLANL
Copy link
Collaborator Author

I fixed the changes (I hope). I don't expect them to be adopted, at this point I just want to get them right as an example.

I think that perhaps what bothers me is the self.outfile propertry. It's in every descendant command class, it gets passed around in so many commands. And it's super limited

  1. It only gets set in the base class. It only changes in response to the -silent flag.
  2. It only has two potential values sys.stdout and io.StringIO().

So here's this pervasive and influential variable that looks like it's set up to be some handle you can write to, and it's really just the default print buffer or nothing, and there's (almost) no way to influence it let alone set it. That seems misleading and unnecessarily verbose from the perspective of the functions calling it.

I'm not sure why I can't reply to comments on this PR.

@dmageeLANL
Copy link
Collaborator Author

It passed! 73e51be shows it can be done. The issue with many of the test failures was that I changed the tests involving -j --json to --full which showed a jsonish output that was not valid json. That flag used pprint.pprint which I think contravenes the pavilion style guide (to use output.) and produces an output that is no more legible than what can be produced by the json library without the ability to pipe the results to jq or a json file, or copied off the terminal into a json file or an ipython instance, etc.

So I'll just leave this here. If you'd like to incorporate some of the ideas, please do, but since you seem reticent to incorporate an outfile flag, I don't expect it to be adopted in total. I'm going to stop working on it now. After I remove the unnecessary changes I did not mean to commit that I made to output.py.

@Paul-Ferrell
Copy link
Collaborator

Paul-Ferrell commented Feb 24, 2022 via email

@Paul-Ferrell
Copy link
Collaborator

Catching up on merge requests.

I'm going to rename --output to --outfile and --errfile, and move them to the base argument parser. That way they'll be valid for and operate on all commands, not just results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants