-
Notifications
You must be signed in to change notification settings - Fork 45
Tools respects pre-existing RAPIDS_USER_TOOLS_LOG_FILE env variable #1895
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
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Pull Request Overview
This PR modifies the tools initialization to respect a pre-existing RAPIDS_USER_TOOLS_LOG_FILE environment variable instead of always overriding it. This allows external libraries or users to specify their own log file location.
Key changes:
- Added logic to check for existing LOG_FILE environment variable before setting a default
- Differentiated messaging between externally configured and default log file locations
- Preserved existing functionality while adding flexibility for external configuration
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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 @sayedbilalbari !
Can we have more details in the description? It is not clear what is the purpose of the change given that users are setting the file names (not directory name)
What is the use case that user manage naming the log file themselves? The idea to link between the log_file and the directory name to know which run produced which logs.
Otherwise, how can someone link between the them?
# LOG_FILE already set by external caller, respect it | ||
log_file = existing_log_file | ||
log_dir = str(Path(log_file).parent) | ||
log_message_prefix = 'Location (External)' |
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.
not very useful message
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, updated to remove this
What I am trying to understand here is what is the context that we give users two options:
The usage is not clear and confusing and there is no documentation on what to expect from the two variables and how they can overlap with each other. |
Thanks @amahussein for the feedback. Some clarification points.
The env variable is the fully qualified path of the log file.
|
Also the other pre-existing logic to set the HOME directory,
it never checks for any pre-existing env variables. So whatever the user sets, cannot override the HOME directory. So no way to configure the directory of the log files. |
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
I am still missing the point.
|
Thanks @amahussein . Very valid points.
Hence the cleanest way without too much confusion was leaving it upto the user to decide the file_name. |
Signed-off-by: Sayed Bilal Bari <[email protected]>
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.
Lets take other devs feedback.
I am going to yield to other devs to vote that up.
@@ -101,6 +101,52 @@ def stringify_path(fpath) -> str: | |||
return os.path.abspath(expanded_path) | |||
|
|||
|
|||
def validate_local_log_file_path(log_file_path: str) -> str: |
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.
Not sure if this is necessary. Seems to be an overkill to validate all those cases for the log-file.
The point that log-file should be very straightforward to log any issues during initializations. Too much complex around the log-files might break before even setting the log-file. which defies the purpose.
Anyway, I am fine with keeping it that way.
print(Utils.gen_report_sec_header('Application Logs')) | ||
print(f'Location: {log_file}') | ||
print(f'Location : {log_file}') |
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.
extra white space before column
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.
Updated, thanks !
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
Signed-off-by: Sayed Bilal Bari <[email protected]>
Thanks @amahussein . Have updated the code to have a common RUN_ID across each run to move the meta information previously contained in the log file name () to the loggers. |
Fixes #1894
Context ->
Changes ->
Testing ->
Log Format changes ->
The updated console logs -
The updated file logs -
Caveats ->