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.

Sign up for GitHub

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

Jump to bottom

Features/logger #69

Merged
merged 6 commits into from
Jun 6, 2024
Merged

Features/logger #69

merged 6 commits into from
Jun 6, 2024

Conversation

koparasy
Copy link
Member

@koparasy koparasy commented Jun 4, 2024

Provide environment variables to log data into files and setup verbosity.

@koparasy koparasy requested a review from lpottier June 4, 2024 22:04
koparasy
koparasy commented Jun 4, 2024
View reviewed changes
tools/AMSLogReader.py Outdated Show resolved Hide resolved
koparasy and others added 2 commits June 5, 2024 08:05
@koparasy
Add limits header for HOST_NAME_MAX def
739be8c
@lpottier
compilation problem: build fails when MPI support is deactivated
11cf71a 
solution: wrapping missing lines of code with __AMS_ENABLE_MPI__.
We have two define __AMS_ENABLE_MPI__ and __ENABLE_MPI__ in the code,
for simplicity I have removed all occurences to __ENABLE_MPI__ and
replaced them by __AMS_ENABLE_MPI__

Signed-off-by: Loic Pottier <pottier1@llnl.gov>
@koparasy
Copy link
Member Author

koparasy commented Jun 5, 2024

@lpottier do you know why our tests did not catch the MPI=Off case? Should I add such a case in our tests?

@lpottier
Copy link
Collaborator

lpottier commented Jun 5, 2024

@lpottier do you know why our tests did not catch the MPI=Off case? Should I add such a case in our tests?

I was wondering the same thing. I checked and there is no tests with WITH_MPI=Off in ci.yml. We should add at least one.

@lpottier
After discussions with @koparasy, reverting all __AMS_ENABLE_MPI__ to…
98a9099 
… __ENABLE_MPI__

Signed-off-by: Loic Pottier <pottier1@llnl.gov>
@koparasy
Copy link
Member Author

koparasy commented Jun 5, 2024

@lpottier do you know why our tests did not catch the MPI=Off case? Should I add such a case in our tests?

I was wondering the same thing. I checked and there is no tests with WITH_MPI=Off in ci.yml. We should add at least one.

I will add one. To make sure this is covered.

lpottier
lpottier requested changes Jun 6, 2024
View reviewed changes
Copy link
Collaborator

@lpottier lpottier left a comment

Choose a reason for hiding this comment

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

It looks good, we just need to fix that segfault when AMS_LOG_DIR is pointing to a directory that does not exist.

src/AMSlib/wf/logger.cpp Show resolved Hide resolved
src/AMSlib/wf/logger.cpp Show resolved Hide resolved
src/AMSlib/AMS.cpp Outdated Show resolved Hide resolved
@lpottier lpottier self-requested a review June 6, 2024 03:34
@koparasy koparasy merged commit f3cc10f into develop Jun 6, 2024
2 checks passed
@koparasy koparasy deleted the features/logger branch June 6, 2024 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpottier lpottier lpottier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koparasy @lpottier