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 logging functionality to InteractiveScriptTemplate.py.template #2282

Merged
merged 9 commits into from Feb 14, 2024

Conversation

samadpls
Copy link
Contributor

resolved #2174
Added log file functionality for Interactive backend

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
Copy link
Member

@egede egede left a comment

Choose a reason for hiding this comment

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

This pull request doesn't seem to do anything. It open a logfile, but there is never anything written to it. The behaviour that we are looking for is that the output after the job has finished should be exactly the same as for the Local backend. So the outputdir should contain the files stdout and stderr with the standard output and error respectively. However, we want it at the same time to be printed to the interactive terminal. So behaviour should be a bit like the tee linux command.

As a secondary comment, it would be good to rewrite the opening and closing of files to use the more modern construction of the with command.

@egede
Copy link
Member

egede commented Feb 12, 2024

There should also be a test case written that shows that it works. Please ask for further advice when getting to that point.

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls samadpls temporarily deployed to Integrate Pull Request February 12, 2024 13:58 — with GitHub Actions Inactive
@samadpls
Copy link
Contributor Author

samadpls commented Feb 12, 2024

Hey, @egede, thanks for your response. I apologize for my mistake, but I have updated the code, and it is now saving output in their respective files and matching the output of the local backend. However, I am still unsure about the correct directory for writing the test cases. Could you please clarify that for me? Thanks.

Edit: One integration test is currently failing and I'm trying to determine the potential reason. It would be helpful if you could also point out the reason for the test case failure.

@samadpls samadpls temporarily deployed to Integrate Pull Request February 12, 2024 14:08 — with GitHub Actions Inactive
@samadpls samadpls requested a review from egede February 12, 2024 14:40
@samadpls samadpls temporarily deployed to Integrate Pull Request February 12, 2024 15:12 — with GitHub Actions Inactive
Copy link
Member

@egede egede left a comment

Choose a reason for hiding this comment

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

This looks good. Please add a test case in https://github.com/ganga-devs/ganga/tree/develop/ganga/GangaCore/test/GPI in a file called TestInteractive.py. Look at a file like https://github.com/ganga-devs/ganga/blob/develop/ganga/GangaCore/test/GPI/TestArgSplitter.py to see how to do ths in an easy way. The test should confirm that both stdout and stderr are created with the correct content.

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
@samadpls samadpls temporarily deployed to Integrate Pull Request February 13, 2024 18:54 — with GitHub Actions Inactive
@samadpls samadpls requested a review from egede February 13, 2024 18:54
egede
egede previously approved these changes Feb 13, 2024
Copy link
Member

@egede egede left a comment

Choose a reason for hiding this comment

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

Great work. Many thanks. This will be merged and made part of the next release.

Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
Copy link
Member

@egede egede left a comment

Choose a reason for hiding this comment

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

This looks good and ready to merge.

@egede egede merged commit 716e7aa into ganga-devs:develop Feb 14, 2024
10 checks passed
@egede
Copy link
Member

egede commented Feb 14, 2024

Please delete the branch your side to avoid pushing to the already merged branch.

@samadpls samadpls deleted the save_log branch February 14, 2024 11:06
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.

Interactive backend should save a log file
3 participants