-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adds the basic logging capabilities for Omega #27
Adds the basic logging capabilities for Omega #27
Conversation
I forgot to apply clang-format. I will update the PR soon after updating code format using clang-format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grnydawn the actual functionality looks good. Most comments are just trying to establish the consistent style for OMEGA. Thanks
@xylar , I got an CI/CD check error as following. Could you look at it? I have no idea how to fix this error: Run trilom/file-changes-action@v1.2.3 |
@philipwjones , while I am having an CI check issue, I think I have done updating logging per suggestions here. Could you check this if it is ready to merge? I have not implemented all of the test cases noted in logging design document yet because I think I will add more tests(especially multi-processing and multi-threading tests) after I finish merging my updates in Cmake-based build. |
@grnydawn, that's nothing you did. It's a CI problem. If I have permission, I'll push a fix to your branch. |
@grnydawn, I don't have permission to push to your branch so I'll make a separate PR to fix this. |
@xylar Thanks for the quick fix. I will try to rebase it. |
@philipwjones , the CI issue is now resolved. I think I have implemented all the change requests in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grnydawn, thanks for the changes and in general for the hard work on this. I'm good with it going in now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grnydawn thanks for the changes. Almost there, but two more little things: First, I'm getting a FAIL in your unit test for YAKL type 1 so CTest is failing. I'm not sure it'll be that common to be logging full YAKL arrays so it's not a huge deal, but we should have a passing unit test before merging. Second, I think the test directory should mirror the src directory, so we should move the logging unit test driver into test/infra (and the path modified accordingly in the Cmake test definition).
@philipwjones , Thanks for the comments. In my ctest, all tests were passed. Could you share "Testing/Temporary/LastTest.log" under the output directory with me? I will move the logging test driver into test/infra. Thanks. |
Here's the output - not much info other than the first YAKL test failed. This was a test I ran on Summit with nvidia. Start testing: Aug 21 18:43 EDT1/1 Testing: LOGGING_TEST
|
@philipwjones Thanks for the info. There was a bug in my log formatter for yakl. I fixed it now and the logging test driver is moved to test/infra now. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes @grnydawn I'll approve now
@philipwjones Thanks. Could you merge this PR to develop. It looks that I am not authorized to merge. |
@grnydawn Can you please rebase to develop? There are some old commits here and when I tried to do the automatic rebase-and-merge, it reported some conflicts. If you can do a rebase, then force push to this branch, that would be great. And if you want to squash commits along the way for an even cleaner PR, that would be even better... Thanks. |
@philipwjones I tried to squash the updates in develop branch to this PR . But as I am not familiar with the git commands, there could be some mistake. I have just pushed the changes and let me know if any changes is needed. |
@grnydawn Sorry if I was unclear or confused things. You needed to do a rebase to develop first to eliminate all the unrelated commits that you had on your branch from other merges so that you only had the commits for the Logging itself. Then after that rebase, do a squash to reduce the number of commits that were only related to this PR. Unfortunately, now you have a bit of a mess because all the unrelated commits are also squashed. Probably the best thing to do now is to create a fresh branch from the latest develop and then cherry-pick the commits that were for this Logging functionality. |
@grnydawn, I have posted detailed instructions for you on Slack of how I would clean this up with a rebase. Please let me know if that works for you or if I can help. |
@philipwjones , sorry for the mess. I will see Xylar's comments on Slack and see what is the best way to fix this. @xylar, Thanks a lot for the details explanations. I will try to follow the instructions. One question: my remote branch of ykim/omega/logging is already filled with unnecessary commits and history. I wonder if you expect me to correct them using your instructions, or are the instructions for the future use? For now, it seems to be something doable for me to create a new branch only for this PR as Phil suggested. |
My instructions are for fixing the existing branch, |
I think it is worth taking the time to do a rebase as I suggested on Slack because this is an important capability for you to have and something you will reuse constantly while you are working on Omega. |
@grnydawn - no worries. Everything in git is fixable and @xylar is more of an expert than I am so hopefully his instructions get you back on track. I will second Xylar's comment that the rebase workflow is really important and it keeps the commit history clean, so no matter how you end up fixing this, getting familiar with rebase is critical. As always, when you get frustrated with git, visit the xkcd comic's take on git: https://xkcd.com/1597/ - especially the alt text you get when you hover over the image... |
Thanks @xylar and @philipwjones . I will try to fix it and see how far I can go ^^ |
This merge add the basic logging capabilities for Omega. These includes: * saving log in a log file in a build directory or to a user specified path. * Logging macros whose name starts with "LOG_" * Multiple logging levels: TRACE, DEBUG, INFO, WARN, WARN, ERROR, and CRITICAL * Custom log formatter for YAKL data types(experimental yet)
be4c9c3
to
b00ada2
Compare
@xylar and @philipwjones , I think I have done the fixing per Xylar's instructions. Hope this works but, otherwise, pls feel free to let me know what I can do. Thanks a lot for the helps. |
The rebase and squash looks good to me. |
Yep, looks good. I'll take another quick look, then go ahead and merge this. Thanks for fixing. |
updating Icepack to hash e751022
This PR add the basic logging capabilities for Omega. These includes:
Checklist
This PR implements only the basic capabilities of logging. Following PR should includes E3SM logging integration, Multi-process support, multi-thread support, buffered/unbuffered logging and more stable YAKL data type support.