Feature/fix long output prints #1513

OlawumiSalaam · 2025-01-16T08:05:05Z

Thank you for taking your time to contribute to Ersilia, just a few checks before we proceed

Have you followed the guidelines in our Contribution Guide
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Description

This PR addresses an issue where large outputs clutter the terminal, making it difficult to interpret results effectively. Specifically, some models, such as descriptors, produce outputs with thousands of columns which floods the terminal and reduce readability.

Changes to be made
Added truncate_output Function:
Handles truncation for large outputs, summarizing them with ellipses (...) and providing context about omitted parts.
Ensures clarity and concise representation of outputs.
Modified Output Handling:
Applied truncate_output to both regular and generator outputs.
Ensured the truncated version is passed to echo or print_result_table as applicable.

Steps taken to implement the fix
Created a utility function, truncate_output, within the output handling module.
Updated the generator loop and regular output sections to conditionally truncate results based on size.
Verified that truncated outputs integrate seamlessly with echo and print_result_table functions.
Status

Implementation complete.
Tested some models: eos4wt0, eos4u6p, eos7w6n, eos4avb, eos3b5e, and eos69p9.
All tests pass successfully when run locally.

To do

Review and address feedback

Is this pull request related to any open issue? If yes, replace issueID below with the issue ID

fixes #1455

DhanshreeA · 2025-01-16T12:21:36Z

ersilia/cli/commands/run.py

@@ -10,6 +10,42 @@
 from . import ersilia_cli


+def truncate_output(output, max_items=10, max_chars=500):


Can we move this function to ersilia/utils/terminal.py?

okay. please what is the logic behind that for learning purpose?

The function you have created is terminal utility, right? So putting it in its own module makes the code clean and managable.

Thanks @Abellegese that is indeed the case, @OlawumiSalaam

@Abellegese Thank you very much for this. I am still learning.

Thats very much appreciated @OlawumiSalaam. Keep learning.

@DhanshreeA
I have moved the truncate_output function to ersilia/utils/terminal.py as suggested and updated the imports.

DhanshreeA · 2025-01-16T12:58:16Z

ersilia/cli/commands/run.py

                else:
                    echo("Something went wrong", fg="red")
        else:
            if as_table:
-                print_result_table(result)
+                if isinstance(result, dict):


Why do we do this? I wouldn't touch this functionality just yet, we need to deal with print_result_table separately.

@DhanshreeA
This is explanation of my logic, if it helps. Let me know if you want me to modify anything

if isinstance(result, types.GeneratorType): for result in mdl.run(input=input, output=output, batch_size=batch_size): if result is not None: truncated = truncate_output(result) # Truncate the output if as_table: print_result_table(truncated) # Print truncated result as table else: echo(truncated) # Print truncated raw output else: echo("Something went wrong", fg="red") # Print error if result is None else: #truncated = truncate_output(result) # Optional: Truncate non-generator output if as_table: print_result_table(result) # Print the entire result as a table else: try: echo(truncated) # Print truncated result except Exception as e: echo(f"Error: {e}", fg="red") # Print error if echo fails print_result_table(truncated) # Fallback: Print truncated result as table

Hi @OlawumiSalaam I mean just try to understand, in this PR you have changed 1 file, run command. I am only seeing you trunctated results in per generator, the other case is not addressed. The print output as table (specifically fixed in my PR) handle printing results from three file types, csv, json and hdf5 also a normal json displayed in the terminal. Maybe you need to create a more general trunctor to handle truncating this files output so that the print table function can display them.

@DhanshreeA @Abellegese
If I understand properly, your suggestion is to extend the functionality of the truncate to handle the other three file types, csv, json and hdf5 so the print table function can display them to?

Yes @OlawumiSalaam. But the files shouldn't be modified because users want them.

DhanshreeA · 2025-01-16T13:01:16Z

@OlawumiSalaam can we write a small unit test for the truncate_output method in a test/test_utils.py file?

OlawumiSalaam · 2025-01-17T12:52:29Z

@OlawumiSalaam can we write a small unit test for the truncate_output method in a test/test_utils.py file?

I have written the unit test. Please review and Please let me know if there is anything else to adjust. Thanks

Abellegese

This is also not clear what the code is testing. So currently you applied truncation for the case the mdl.run returns generator. This generator always returns a json based output from the conventional runner, given as below:

[
{
    "input": {
        "key": "NQQBNZBOOHHVQP-UHFFFAOYSA-N",
        "input": "C1=C(SC(=N1)SC2=NN=C(S2)N)[N+](=O)[O-]",
        "text": "C1=C(SC(=N1)SC2=NN=C(S2)N)[N+](=O)[O-]"
    },
    "output": {
        "outcome": [
            261.313
        ]
    }
}
{
    "input": {
        "key": "HEFNNWSXXWATRW-UHFFFAOYSA-N",
        "input": "CC(C)CC1=CC=C(C=C1)C(C)C(=O)O",
        "text": "CC(C)CC1=CC=C(C=C1)C(C)C(=O)O"
    },
    "output": {
        "outcome": [
            206.28499999999997
        ]
    }
}
]

In the output: { outcome section there are several datastructure currently supported by Ersilia such as given below. In the above output case, the datastructure is Single.

{
    "Single": lambda x: isinstance(x, list) and len(x) == 1,
    "List": lambda x: isinstance(x, list)
    and len(x) > 1
    and all(isinstance(item, (str, int, float)) for item in x),
    "Flexible List": lambda x: isinstance(x, list)
    and all(isinstance(item, (str, int, float)) for item in x),
    "Matrix": lambda x: isinstance(x, list)
    and all(
        isinstance(row, list)
        and all(isinstance(item, (str, int, float)) for item in row)
        for row in x
    ),
    "Serializable Object": lambda x: isinstance(x, dict),
}

So I believe you should somehow make the truncation function to be able to work with them. Also in the unittest it there should be a test to test this structures based output.

Abellegese · 2025-01-20T11:40:15Z

ersilia/utils/terminal.py

+    elif isinstance(output, str):
+        if len(output) > max_chars:
+            suffix = " ... (truncated)"
+            truncated = output[
+                : max_chars - len(suffix) - 1
+            ].rstrip()  # Adjust for space
+            return f"{truncated} {suffix}"
+        return output


Hi @OlawumiSalaam can you explain the case this code truncates or gets used?

@Abellegese
Thank you for your detailed feedback.
This is my first task where I am diving deeper into the Ersilia codebase, and I am still in the process of fully understanding the purpose and structure of each part of the code. Initially, I wasn’t certain whether all models produced the same type of output, so I aimed to create a generic truncation function that could handle common data structures.

if I understand you clearly now, i should modify the function to handle the supported data structures (e.g., Single, List, Matrix, Serializable Object) as shared from the feedback.

DhanshreeA · 2025-01-21T07:13:41Z

@OlawumiSalaam This test is failing on your branch:

Traceback (most recent call last):
File "/usr/share/miniconda/envs/test/bin/ersilia", line 8, in
sys.exit(cli())
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/click/core.py", line 1161, in call
return self.main(*args, **kwargs)
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/click/core.py", line 1082, in main
rv = self.invoke(ctx)
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/click/core.py", line 1697, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/click/core.py", line 1443, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/click/core.py", line 788, in invoke
return __callback(*args, **kwargs)
File "/home/runner/work/ersilia/ersilia/ersilia/cli/commands/init.py", line 31, in wrapper
return func(*args, **kwargs)
File "/home/runner/work/ersilia/ersilia/ersilia/cli/commands/run.py", line 79, in run
echo(truncated)
File "/home/runner/work/ersilia/ersilia/ersilia/cli/echo.py", line 85, in echo
text = emoji.emojize(text)
File "/usr/share/miniconda/envs/test/lib/python3.10/site-packages/emoji/core.py", line 219, in emojize
return pattern.sub(replace, string)
TypeError: expected string or bytes-like object

truncated is supposed to return text output. I believe that's not what's happening with your recent changes causing the echo function to fail.

OlawumiSalaam · 2025-01-21T07:17:27Z

@DhanshreeA I will push the updated changes

OlawumiSalaam · 2025-01-21T11:34:12Z

@DhanshreeA @Abellegese
Thank you for you help. I have addresses the feedbacks. Please kindly review the updated task. Thank you.

OlawumiSalaam · 2025-01-21T11:50:36Z

Hi @DhanshreeA
I am struggling with my current task but I have updated the PR based on your feedback and Abel's feedback. Abel was on a call with me yesterday and he was very helpful. Although, I am working on the unit test to test the functionality. Work is a bit slow due to internet issues. I have pushed what I did so far. I want to let you know what is happening at my end.
As soon as I finish the unit test, I will update you and push it to the PR
Thank you

DhanshreeA · 2025-01-21T13:03:45Z

@OlawumiSalaam I think the confusion should resolve once I merge @Abellegese's PR since both of you are working on terminal utilities, specifically those that go within the run command.

DhanshreeA · 2025-01-21T13:56:25Z

@OlawumiSalaam could you look into the conflicts on this PR? I would first recommend checking out the master branch, seeing how the code works especially with the --as_table functionality now working, and then taking it from there. Specifically, I would suggest doing the following:

Run a few different models as ersilia run -i "<some input>" --as_table, or even as `ersilia run -i "" -o "output.csv" --as_table"
You can experiment both with a couple of different models, such as eos3b5e, eos7w6n, eos5guo etc, and with different output formats (csv, json, h5)
Then figure out how the truncate_output function would fit into this picture, especially in the case of when an output file is requested, this could mean, using your function INSIDE the print_result_table function, as opposed to passing it truncated values from your function.

OlawumiSalaam · 2025-01-22T11:58:53Z

@DhanshreeA
I have seen the way print_result_table works on the terminal. even though a table is rendered, the terminal is still clustered and not very easy to follow. When an output file is specified (e.g., -o "output.csv"): Should truncation still apply here? Or should it only apply to the terminal display?

GemmaTuron · 2025-01-22T12:27:43Z

Hi @DhanshreeA and @OlawumiSalaam

I think there is a misunderstanding here. This is not what I meant by fixing the long output printing. I do not want to modify anything on the CLI, only the logger.DEBUG
I am afraid we are introducing bugs here that we do not want. The terminal should always print the entire output upon the "run" command when no output is specified. Please do not modify anything about how the terminal is currently showing the outputs, not even -as-table or anything like that

OlawumiSalaam · 2025-01-22T12:48:56Z

@GemmaTuron
This is well understood.

DhanshreeA · 2025-01-22T12:58:58Z

Closing this PR as we were working on the wrong thing all along. @OlawumiSalaam will create a new PR.

OlawumiSalaam added 6 commits January 16, 2025 08:31

Fix long output prints

d818949

format long output

730f43e

fix long output

1948084

fix long output

cb87451

fix long output print

19b8aed

Fix long print outputs and format code with pre-commit hooks

a255ce5

DhanshreeA requested changes Jan 16, 2025

View reviewed changes

unit test for the truncate_output method

dab9980

unit test for truncate_output function

afdc23d

Abellegese reviewed Jan 20, 2025

View reviewed changes

Improved data structures and file type handling

cb74572

DhanshreeA closed this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fix long output prints #1513

Feature/fix long output prints #1513

OlawumiSalaam commented Jan 16, 2025

DhanshreeA Jan 16, 2025

OlawumiSalaam Jan 16, 2025

Abellegese Jan 17, 2025

DhanshreeA Jan 17, 2025

OlawumiSalaam Jan 17, 2025

Abellegese Jan 17, 2025

OlawumiSalaam Jan 17, 2025

DhanshreeA Jan 16, 2025

OlawumiSalaam Jan 16, 2025

Abellegese Jan 17, 2025

OlawumiSalaam Jan 17, 2025

Abellegese Jan 17, 2025

DhanshreeA commented Jan 16, 2025

OlawumiSalaam commented Jan 17, 2025 •

edited

Loading

Abellegese left a comment •

edited

Loading

Abellegese Jan 20, 2025

OlawumiSalaam Jan 20, 2025 •

edited

Loading

OlawumiSalaam Jan 20, 2025

DhanshreeA commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

DhanshreeA commented Jan 21, 2025

DhanshreeA commented Jan 21, 2025

OlawumiSalaam commented Jan 22, 2025

GemmaTuron commented Jan 22, 2025 •

edited

Loading

OlawumiSalaam commented Jan 22, 2025

DhanshreeA commented Jan 22, 2025

		@@ -10,6 +10,42 @@
		from . import ersilia_cli


		def truncate_output(output, max_items=10, max_chars=500):

Feature/fix long output prints #1513

Feature/fix long output prints #1513

Conversation

OlawumiSalaam commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhanshreeA commented Jan 16, 2025

OlawumiSalaam commented Jan 17, 2025 • edited Loading

Abellegese left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlawumiSalaam Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhanshreeA commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

OlawumiSalaam commented Jan 21, 2025

DhanshreeA commented Jan 21, 2025

DhanshreeA commented Jan 21, 2025

OlawumiSalaam commented Jan 22, 2025

GemmaTuron commented Jan 22, 2025 • edited Loading

OlawumiSalaam commented Jan 22, 2025

DhanshreeA commented Jan 22, 2025

OlawumiSalaam commented Jan 17, 2025 •

edited

Loading

Abellegese left a comment •

edited

Loading

OlawumiSalaam Jan 20, 2025 •

edited

Loading

GemmaTuron commented Jan 22, 2025 •

edited

Loading