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

Add new functions and update print_result #21

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

timeforplanb123
Copy link

@timeforplanb123 timeforplanb123 commented Nov 2, 2021

update print_result function:

  • add new count parameter. It's only to limit the amount of output to the terminal without changing the scrollback terminal settings, but at the same time to see some results. I think, it's useful, at least, for nornir_cli tool
  • add example in docs/source/tutorials/function_print_result.ipynb

add new print_stat function:

  • function prints statistic for result
  • recursive function, that will correctly process any AggregatedResult, MultiResult, Result objects and skip non-AggregatedResult, non-MultiResult, non-Result objects
  • add docs/source/tutorials/function_print_stat.ipynb with examples

add new write_result function:

  • function writes result to file
  • write_result returns diff between previous file state and new file state (str)
  • add docs/source/tutorials/function_write_result.ipynb with examples

add new write_results function:

  • function writes result to files with hostname names
  • write_results returns list of tuples with hostname + diff between previous file state and new file state
  • add docs/source/tutorials/function_write_results.ipynb

This PR is instead of closed PR: #15 #16 #17 #18

@timeforplanb123
Copy link
Author

timeforplanb123 commented Mar 27, 2022

a small update:

  • fix flake8 max-line-length to 88 from 79
  • merge with master branch
  • add missing a return type annotation to print_result.py
  • fix new functions .ipynb tutorials

@dbarrosop
Copy link
Contributor

I just say that I do really appreciate the work here but everything we discussed in #15 #16 #17 #18 still applies. It is also not good etiquette to send a single PR with many new and unrelated features and fixes. It makes the review process very difficult and increases the chances the PR won't make it. For instance, I don't think I will be accepting the count feature. The rest seem fine in principle but I haven't reviewed since the last round of reviews in the old PRs due to the size of this PR.

@timeforplanb123
Copy link
Author

I apologize for this PR, and I also really don't like PR of this size, and I really tried to make it convenient for the PR reviewer. But print_state, write_result, write_results had a dependency on the new _slice_result function from print_result (yes, this is for the count parameter), so I divided this PR into #15, #16, #17, #18, and then moved them to #21, but, in the first case, it was badly and incomprehensibly, and, in the second case - large size PR :(

I also already think, that count doesn't make sense, so I deleted count and everything related to it. At the same time, I decided to keep this PR, because it would not be very convenient to merge nornir_utils.plugins.functions.__init__ (tutorials depend on it) and README.rst manually.
So, now there are only 3 new functions (print_stat, write_result, write_results + .ipynb tutorials).

And... I didn't really understand, why the tests, related to other modules, were failing... I would be very glad, if you could help me with this:

FAILED docs/source/tutorials/echo_data.ipynb::Cell 0
FAILED docs/source/tutorials/function_print_result.ipynb::Cell 1
FAILED docs/source/tutorials/function_print_result.ipynb::Cell 2
FAILED docs/source/tutorials/function_print_result.ipynb::Cell 4
FAILED docs/source/tutorials/function_print_result.ipynb::Cell 5
FAILED docs/source/tutorials/function_print_title.ipynb::Cell 0
FAILED docs/source/tutorials/tcp_ping.ipynb::Cell 0
FAILED docs/source/tutorials/write_file.ipynb::Cell 0

- print_stat - Prints results statistic on stdout.
- write_result - Writes results to file.
- write_results - Writes results to files.
@dbarrosop
Copy link
Contributor

Ok, sounds good. Given that everything is new functionality now I am less concerned about the PR. I will try to put aside some time for this but these days time is very scarce, so apologies if this takes a while.

LOCK = threading.Lock()


init(autoreset=True, strip=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is why the tests are failing, we are initializing colorama twice. Remove this line and the tests will all pass (you will need to fix your tests)

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move the other init call to init to have it in a central place

Copy link
Author

Choose a reason for hiding this comment

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

moved colorama init to __init__ central place

from nornir.core.task import AggregatedResult, MultiResult, Result


LOCK = threading.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use the same lock that print_result uses, otherwise, print_result and print_stat can interfere with each other

Copy link
Author

Choose a reason for hiding this comment

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

imported print_result LOCK for print_stat function

@@ -0,0 +1,220 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a test printing all kind of objects directly. Right now you are printing an AggregatedResult in all the examples in here so it'd be nice to print directly the other types there (i.e. print_stats(results["my_device"]) and print_stats(results["my_device"][0])

Copy link
Author

Choose a reason for hiding this comment

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

Added MultiResult and Result objects to function_print_stat.ipynb tutorial

@dbarrosop
Copy link
Contributor

I like the functionality behind write_result and write_results but I think we should implement it slightly different to avoid having two different functions that format the results similarly but implement different functionality.

So I think what we could do instead is the following:

  1. Modify print_result to accept a parameter of type typing.IO that works the same way as the parameter file for the print method:
# this is pseudo-code
def print_result(...., file: Optional[typing.IO] = None):
    # if print gets `None` it defaults to sys.stdout
    print(..., file=file)

We may need to add another parameter to tell colorama to not add escape codes for colors, not sure what the best way of doing that would be though but I am happy to ignore this issue for now.

  1. Now we could write write_results_to_file(results: Union[Result, AggegatedResult, MultiResult], filepath: pathlib.Path) that wraps around print_result and _generate_diff to write and diff one file and write_results_to_directory(results: Union[Result, AggegatedResult, MultiResult], directory: pathlib.Path) to write to many files and diff them. These are basically your write_result and write_results but without any logic to format the results, instead, they can leverage print_result.

Thoughts?

- Move colorama init to __init__ central place
- Use the print_result LOCK for print_stat
- Add MultiResult and Result objects to function_print_stat.ipynb tutorial
- Make the code cleaner
@timeforplanb123
Copy link
Author

I apologize for such a long pause in this PR and thank you so much for your help with testing.

I thought for a long time... and to me, as a user, it seems inconvenient to be able to write to a file in one function (print_result, as instance), and the possibility of formatting in another function (write_result / write_results). If I am writing to a file, I would like to be able to format here and now, and not use a separate function for this.

_print_result, _write_result, _write_results implement similar functionality, but i think, it's wrong to add the functionality of writing to a file/directory in print_result, both from the user's point of view and from the developer's point of view. This will make it more difficult for the user to understand/use the function and for the developer to understand/read the code.

I see 2 options here:

  1. Write a separate module with a service function with the necessary set of parameters (_print_write_result, as an example), which will be used by all user functions (print_result, write_result, write_results)
  2. Keep the functionality, implemented using the service functions _print_result, _write_result, _write_results unchanged. This is also good, because each function is a recursive function, that returns only the data, that is required inside a specific module. Adding parameters and code, used by another module, will complicate the understanding and readability of the code.

I have kept here option 2 for further discussion

@dbarrosop
Copy link
Contributor

dbarrosop commented Aug 8, 2022

If I am writing to a file, I would like to be able to format here and now, and not use a separate function for this

I am not sure I understand this. Why can't you do that in my proposal?

but i think, it's wrong to add the functionality of writing to a file/directory in print_result

What I proposed is to have print_result behave exactly the same way the builtin function print works, which it can write to a file.

I think you misunderstood my proposal. In my proposal you don't need to call two functions and you don't need to implement anything that isn't pythonic. You'd have print_result behave similar to the builin print and write other functions that implement your logic and simply leverage print_result to avoid having to duplicate all that logic. Sorta like (pseudocode-ish):

def write_to_files(result: AggregatedResult) -> None:
     for hostname, r in result.items():
           with open(hostname, "w+") as f:
                   print_result(r, file=f)

- Add file, no_errors, print_host parameters to print_result function
- Update print_stat output format
- write_result, write_results are based on print_result function
- Remove diff feature from write_result and write_results functions
- Update tutorials
- Import reorder
- Change docstring
@timeforplanb123
Copy link
Author

Oh! I really misunderstood this great proposal and this is really the best way! Thank you very much for the idea :)

So, now in this PR:

  1. print_result:
  • added a file parameter. The file argument must be an object with a write(string) method; if it is not present or None, sys.stdout will be used. The file argument of the print_result function uses the same file argument of the builtin print function
  • added no_errors parameter. If some Result objects have exceptions, then you can exclude them from the output of the print_result function. I think this is a useful option that is also used by the write_result and write_results functions
  • added print_host parameter. write_result and write_results functions are based on print_result function with file argument, so the print_host argument can be used by them, for example, when there is already hostname in the filename and you don't want to duplicate it
  • import reorder with flake8-import-order
  1. print_stat:
  • updated print_stat output format
  • updated tutorial (there are examples here for AggregatedResult, MultiResult, Result objects)
  • import reorder with flake8-import-order
  1. write_result:
  • now this is a simple function, that writes result to file. This function is based on print_result function with file argument
  • there are no_errors and write_host parameters, that use no_errors and print_host arguments from the print_result function
  • updated tutorial
  • import reorder with flake8-import-order
  1. write_results:
  • now this is a simple function, that writes result to files with hostname names. This function is based on print_result function with file argument
  • there are no_errors and write_host parameters, that use no_errors and print_host arguments from the print_result function
  • updated tutorial
  • import reorder with flake8-import-order

In addition, it has already been done:

  • moved colorama init to __init__ central place
  • imported print_result LOCK for print_stat function

I also removed the diff feature from the write_result and write_results functions because it is redundant here.

During testing, an error has occured with installing the latest version of poetry (1.2.0) using install-poetry action. I have created a issue #32 for this and PR #33 as a solution

@timeforplanb123
Copy link
Author

A small update based on #35:

  • merged with master branch
  • updated type hints in print_result.py, write_result.py, write_results.py

@timeforplanb123
Copy link
Author

The workflow does not work correctly with python 3.7 (example - https://github.com/timeforplanb123/nornir_utils/actions/runs/6573793757/job/17857529111). I have opened a separate #37 PR to remove the python 3.7

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