Skip to content

Conversation

@bendhouseart
Copy link
Collaborator

I don' think this will be as easy as I would like, e.g. I think it's going to require adding in this to each file in the R folder:

MH02276145MLI:kinfitr galassiae$ tree R
R
├── kinfitr_1tcm.R
├── kinfitr_2tcm.R
├── kinfitr_2tcm1k.R
├── kinfitr_2tcm_irr.R
├── kinfitr_2tcm_macro.R
├── kinfitr_SIME.R
├── kinfitr_SUV.R
├── kinfitr_bids.R
├── kinfitr_blooddata.R
├── kinfitr_bloodfuncs.R
├── kinfitr_bloodmodels.R
├── kinfitr_bloodstream.R
├── kinfitr_data_oldbids_json.R
├── kinfitr_data_pbr28.R
├── kinfitr_data_simrefdata.R
├── kinfitr_feng_1tc_ref.R
├── kinfitr_frtm.R
├── kinfitr_lin2tcm.R
├── kinfitr_loganplot.R
├── kinfitr_ma1.R
├── kinfitr_ma2.R
├── kinfitr_methods.R
├── kinfitr_miscfuncs.R
├── kinfitr_mlloganplot.R
├── kinfitr_mrtm1.R
├── kinfitr_mrtm2.R
├── kinfitr_patlakplot.R
├── kinfitr_pfmodels.R
├── kinfitr_refPatlak.R
├── kinfitr_reflogan.R
├── kinfitr_refmllogan.R
├── kinfitr_srtm.R
├── kinfitr_srtm2.R
├── kinfitr_srtm_v.R
├── kinfitr_telemetry.R
├── kinfitr_weights.R
├── kiniftr.R
└── utils-pipe.R

0 directories, 38 files

But I could be wrong on that.

@bendhouseart bendhouseart linked an issue Jan 16, 2025 that may be closed by this pull request
@mathesong
Copy link
Owner

These look great. I'll help to tidy up the code a little and make it a bit more R-y next week, but in principle having it as a couple of generic functions is perfect.

There's no need to define the telemetry in every function, especially because these can sometimes be run 500 times in a row, and we might overload people's internet connections and your server. I think the way to go will be to execute it on load of the package. Info here: https://stackoverflow.com/questions/20223601/r-how-to-run-some-code-on-load-of-package

@mathesong
Copy link
Owner

And more here. This is the definitive R package book: https://r-pkgs.org/code.html

@bendhouseart bendhouseart changed the base branch from master to Development January 17, 2025 15:18
@bendhouseart
Copy link
Collaborator Author

These look great. I'll help to tidy up the code a little and make it a bit more R-y next week, but in principle having it as a couple of generic functions is perfect.

There's no need to define the telemetry in every function, especially because these can sometimes be run 500 times in a row, and we might overload people's internet connections and your server. I think the way to go will be to execute it on load of the package. Info here: https://stackoverflow.com/questions/20223601/r-how-to-run-some-code-on-load-of-package

That seems much more manageable!

@bendhouseart bendhouseart marked this pull request as ready for review January 24, 2025 22:17
@bendhouseart
Copy link
Collaborator Author

bendhouseart commented Jan 24, 2025

Okay, it's nearly there. only issue I see is that on build it seems to post 3 times to it's endpoint. I think it should only do this once and after that's the case we just need to switch the url endpoints to our actual tracking server. See outputs here -> http://54.144.240.214/check/kinfitr/

Right now it's posting to dev.

We also might want to come up with a more clever way to silence the message about tracking.

── R CMD build ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
✔  checking for file/home/anthony/Projects/kinfitr/DESCRIPTION...preparingkinfitr:checking DESCRIPTION meta-information ...checking for LF line-endings in source and make files and shell scriptschecking for empty or unneeded directories
     NB: this package now depends on R (>= 3.5.0)
     WARNING: Added dependency on R >= 3.5.0 because serialized objects in
     serialize/load version 3 cannot be read in older versions of R.
     File(s) containing such objects:kinfitr/data/oldbids_json.RData’ ‘kinfitr/data/pbr28.RData’
       ‘kinfitr/data/simref.RData’
─  buildingkinfitr_0.8.0.tar.gzRunning /usr/lib/R/bin/R CMD INSTALL /tmp/RtmpFNlekp/kinfitr_0.8.0.tar.gz \
  --install-tests 
* installing to library/home/anthony/.local/lib/R* installing *source* packagekinfitr...
** using staged installation
** R
** data
*** moving datasets to lazyload DB
** tests
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
Loading kinfitr
new debug message
Opt-out of sending tracking information to the KinFitR developers. This information provides an indicator of real world usage crucial for obtaining funding. To disable tracking set options(kinfitr.no_track = TRUE) or the environment variable KINFITR_NO_TRACK to TRUE), to hide this message set options(kinfitr.no_track = FALSE) or the environment variable KINFITR_NO_TRACK to FALSE
Sending telemetry
Debug: Starting send_telemetry
Debug: Telemetry data: package_loaded
Debug: KINFITR_NO_TRACK value: 
Debug: Attempting to send telemetry
Debug: Sending to URL: http://54.144.240.214/kinfitr/
Debug: Response status: 200
** testing if installed package can be loaded from final location
Loading kinfitr
new debug message
Opt-out of sending tracking information to the KinFitR developers. This information provides an indicator of real world usage crucial for obtaining funding. To disable tracking set options(kinfitr.no_track = TRUE) or the environment variable KINFITR_NO_TRACK to TRUE), to hide this message set options(kinfitr.no_track = FALSE) or the environment variable KINFITR_NO_TRACK to FALSE
Sending telemetry
Debug: Starting send_telemetry
Debug: Telemetry data: package_loaded
Debug: KINFITR_NO_TRACK value: 
Debug: Attempting to send telemetry
Debug: Sending to URL: http://54.144.240.214/kinfitr/
Debug: Response status: 200
** testing if installed package keeps a record of temporary installation path
* DONE (kinfitr)
Loading kinfitr
new debug message
Opt-out of sending tracking information to the KinFitR developers. This information provides an indicator of real world usage crucial for obtaining funding. To disable tracking set options(kinfitr.no_track = TRUE) or the environment variable KINFITR_NO_TRACK to TRUE), to hide this message set options(kinfitr.no_track = FALSE) or the environment variable KINFITR_NO_TRACK to FALSE
Sending telemetry
Debug: Starting send_telemetry
Debug: Telemetry data: package_loaded
Debug: KINFITR_NO_TRACK value: 
Debug: Attempting to send telemetry
Debug: Sending to URL: http://54.144.240.214/kinfitr/
Debug: Response status: 200
R

R version 4.4.2 (2024-10-31) -- "Pile of Leaves"
Copyright (C) 2024 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

@mathesong
Copy link
Owner

I took a little look now, but I can try to find some time to merge and edit on Monday.

Firstly, regarding the running three times, I think it's because it's installing. With just attaching it with library(kinfitr), it won't do the testing the installation paths. So then I think it will only run once.

Secondly, to get it to work properly on installation too, I think we could borrow some code from here: there seems to be a session flag that we can take advantage of, or even set our own.

Third: I'm not happy with having to change to LazyLoad: no in the DESCRIPTION file. I think there's an issue someplace that's causing it not to work without that. I'll see if I can get rid of the issue. Some clues here: https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/lazyLoad . I think this might have something to do with the very prescribed nature of R packages (and devtools and usethis have functions to simplify this): I've just not bumped into this issue before because I usually use those packages for building packages.

Fourth: I guess the makefile is there for testing? Or was the idea to leave it?

get_telemetry <- function(url = get_url, number_of_records = 0) {
# checks to see what's been posted to the url endpoint, should return location,
# and any other data that gets put there with send_telemetry
req <- request(paste(url, as.character(number_of_records), sep = ""))
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason you didn't chain these together? I've not used httr2 before, but usually these kinds of things would be chained together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's because I'm writing things like C or Python functions and not R functions. I'm also wrapping them into line by line b/c my R linter is screaming at me if I go over 80 characters.

DESCRIPTION Outdated
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
LazyLoad: no
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not 100% on what LazyLoad is, but I'm really hesitant for it to be altered...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what it does either, but I'm planning on reverting this before we merge.

Switch import() to specific httr2:: syntax
@@ -0,0 +1,42 @@
# set url_base to http://openneuropet.org for deployment
url_base <- "http://54.144.240.214"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
url_base <- "http://54.144.240.214"
url_base <- "http://openneuropet.org"

Apply this suggestion before merge!

@mathesong
Copy link
Owner

I looked this through and made some changes for readability and style. I'm just a little bit particular about a few things.

However, when I make this, there appear to be 6 new entries at the URL endpoint each time, which seems like a lot... I don't think I've added any issues. I tried backtracking on some of the things I was a little concerned about (I don't like the %>% as.logical() syntax ), but it's still doing the same thing. So maybe let's look at this together.

@bendhouseart
Copy link
Collaborator Author

Check to see if this crashes when writing the json file to a root owned folder, when run as non-root user.

@mathesong mathesong merged commit bfe2535 into Development Feb 19, 2025
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.

Adding telemetry KinFitR

3 participants