Skip to content

Conversation

BardyBard
Copy link
Collaborator

Unnecessary dependencies in little jar that were unused as mentioned by Marcus.

@rafapereirabr
Copy link
Member

This PR is not necessary anymore ?

@BardyBard BardyBard reopened this Mar 23, 2025
@BardyBard
Copy link
Collaborator Author

BardyBard commented Mar 23, 2025

@rafapereirabr PR wasn't passing last night, passes locally now, will check github tests in the morning.

JRI imports we're useful contrary to what Marcus and I originally thought, they allowed for writing tests within R that could read the java outputs and verify that verbose and progress were working properly. However, I still wanted to decouple the little jar from being dependent on an entire R engine.

To solve this I have configured the java logger to save log output to a r5r-log.log in the working directory, on top of printing to the console. The R test is then able to confirm the log is correct from this file. This now doubles as a quality of life feature for easier log sharing and bug reporting, or if R studio crashed for some reason and you weren't able to save the log.

In the future the log files should have session id's so that they don't overwrite eachother but that is beyond the scope of this PR I think as this issue was never a priority I just got sucked into it and didn't want to give up.

@rafapereirabr
Copy link
Member

@BardyBard , can we confugure this so that r5r-log.log is saved at the data path of the user? ps. a simple way to avoid overwritting the log file would be to use a time stamp in the file name consideting the time the file is written . E.g r5r_log_20250324173321.log for a file that was writting on the year 2025, month 03, day 24 and time 17h33m21s

@rafapereirabr rafapereirabr mentioned this pull request Mar 24, 2025
@BardyBard
Copy link
Collaborator Author

BardyBard commented Apr 9, 2025

@rafapereirabr Log file now gets saved to the datapath. File is called r5rlog_date.log. New log file gets created each calendar day and all logs from that day get appended to this file. I tried using unique files for each runtime but that fills up the directory and gets annoying very fast.

Theres just one issue with this implementation: I can only pass information to the logger through env variables and I can only pass those from R before .jinit() is called. Once .jinit() is called once, calling it again doesn't reinitialize java and so I am unable to redefine a new logging path if the user changed their datapath mid session, until R studio is restarted.

@rafapereirabr
Copy link
Member

this issue with changing the data path is nor suuuper critical, but it is annoying. I wouldn't make sense to pass the text of the log report from Jave to R and then we save it using R, right? Because if Java crashes, we would not be able to pass the report. correct ?

@BardyBard
Copy link
Collaborator Author

I have not had a lot of success being able to pass the logs from Java to R and still be able to manipulate them aside from printing to the console (my knowledge of R is non-existent so I rely on things chatgpt tells me to try when it comes to manipulating the R side).

Previously, information was passed from Java to R using JRI (Java R interface). This was used only in one place, to verify that progress=True and verbose=True was working properly. When I removed the JRI dependency from the Java side I was no longer able to make these tests work, which is where the logging to a text file idea originated from, to still be able to run this test.

Since all the logs still get printed to the R studio console, and this text file is mostly a workaround for the testthat test, I don't think it is critical to make the datapath changeable. Interestingly running R5R using the check button in Rstudio and the console seems to be different terminals as it re-initiates rJava and passes through the new datapath.

@BardyBard BardyBard merged commit 4f580c4 into master Apr 10, 2025
@BardyBard BardyBard deleted the alex-removeJRI branch April 26, 2025 21:23
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.

2 participants