cytoscape / RCy3

New version of RCy3, redesigned and collaboratively maintained by Cytoscape developer community
MIT License
49 stars 20 forks source link

Do we need to add "setDefaultBaseUrl" function to RCy3? #69

Closed kozo2 closed 3 years ago

kozo2 commented 4 years ago

Some users may want to run RCy3 from Bioconductor Docker RStudio or WSL2. In such case, I think we need to add setDefaultBaseUrl function to RCy3. (Becase writing base.url = option every time is bothersome.) What do you think about this?

AlexanderPico commented 4 years ago

I would need to see a demonstration of this. I didn't know it was possible! The use case I know of would have RStudio and Cytoscaped installed in the same VM, so 'localhost' would be correct.

AlexanderPico commented 4 years ago

But, regardless of the Docker use case, I think you're probably correct, that setDefaultBaseUrl would be helpful.

AlexanderPico commented 4 years ago

Wait, that's why .defaultBaseUrl exists :)

Users can set default base url with a simple assignment; no function needed, e.g.,

.defaultBaseUrl <- "http://localhost:4321"

kozo2 commented 4 years ago

I think your example

.defaultBaseUrl <- "http://localhost:4321"

does not work.

The demonstration of this is

> library(RCy3)
Registered S3 method overwritten by 'R.oo':
  method        from       
  throw.default R.methodsS3
> cytoscapePing()
Error in curl::curl_fetch_memory(url, handle = handle) : 
  Failed to connect to localhost port 1234: Connection refused
> .defaultBaseUrl <- "http://172.18.176.1:1234/v1"
> cytoscapePing()
Error in curl::curl_fetch_memory(url, handle = handle) : 
  Failed to connect to localhost port 1234: Connection refused
> cytoscapePing(base.url = "http://172.18.176.1:1234/v1")
[1] "You are connected to Cytoscape!"
kozo2 commented 4 years ago

And these commands also do not work

> RCy3:::.defaultBaseUrl <- "http://172.18.176.1:1234/v1"
Error in RCy3:::.defaultBaseUrl <- "http://172.18.176.1:1234/v1" : 
  object 'RCy3' not found
AlexanderPico commented 4 years ago

This turned out to be more interesting than I had thought... You are correct that setting the value of .defaultBaseUrl does not work. But that also means that a having a function to set it also doesn't work. Since the default is defined in the package, it's local value is ignored.

Of course, there is a package for cases just like these and it's called default: https://cran.r-project.org/web/packages/default/index.html

To change the default value of a package function, you would do this:

default(cytoscapePing) <- list(base.url="http://172.18.176.1:1234/v1")

And you can restore package defaults with this:

cytoscapePing()<-reset_default(cytoscapePing)

You have to run this for each function you want to change.

AlexanderPico commented 4 years ago

Not sure which is easier...

  1. Providing the base.url for every function call, or
  2. Setting a new local default for every function.

I guess it will depend on the nature of the script and the user's preference.

yangli-ai commented 3 years ago

If you change "base.url=.defaultBaseUrl" into "base.url=defaultBaseUrl" for every function, it works. So why not change this?

kozo2 commented 3 years ago

@229668880 Thank you for your comment. But I don't fully understand what your comment means. Could you give me the full code example?

yangli-ai commented 3 years ago

I use RCy3 in R REPL in one Windows PC. I try to connect R to the Cytoscape in the other Windows PC. All functions in RCy3 have a parameter base.url=.defaultBaseUrl, but this parameter doesn't work even if the variable .defaultBaseUrl is already set in R REPL. Maybe changing .defaulBaseUrl into defaultBaseUrl can work. Take cytoscapeVersion(Base.url=.defaultBaseUrl) for example.

.defaultBaseUrl='http://192.168.1.1/v1' defaultBaseUrl='http://192.168.1.1/v1' cytoscapeVersionInfo2<-function(base.url=defaultBaseUrl){ cytoscapeVersionInfo(base.url=base.url) }

cytoscapeVersionInfo(.defaultBaseUrl) # this doesn't work cytoscapeVersionInfo2(defaultBaseUrl) # this works

So, do all functions should be changed to take defaultBaseUrl intead of .defaultBaseUrl?

kozo2 commented 3 years ago

@229668880 What I'm looking for seems to be different from what you say. What I want to achieve is to eliminate the need to write base.url=***.***.***.*** (or argument such as defaultBaseUrl) in RCy3 functions once the defaultBaseUrl or.defaultBaseUrl is set. I think your example needs to write defaultBaseUrl in RCy3 functions every time.

AlexanderPico commented 3 years ago

Ok. I learned a new trick that might be relevant here.

We added internal variables for delays last release and there was a request to provide a function to make this adjustable by the user. My first thought was, "oh no, another parameter is a bunch of functions." But I ended up learning how to define a cache environment where we can assign and get internal package variables. It appears to be working well and should be able to work here too.

Scenario 1: default base URL. Nothing would change for this common user scenario. A default value for .BASE_URL will be assigned when the package is loaded.

Scenario 2: custom base URL. I can expose a function called setBaseUrl(url=***). The user can call this once and it will work for all functions in the package. The user can call the function without an arg to reset to the default.

I believe this is what @kozo2 and @229668880 (above) originally wanted, but I didn't not know to implement this before. I will give it a try in a branch. Can you help test?

Example of variable assignment, usage and setter function.

kozo2 commented 3 years ago

@AlexanderPico Thank you for the information. Yes, I would like to help test. Are you planning to add (or change) the code below?

RCy3-utils.R

assign(".defaultBaseUrl", 'http://localhost:1234/v1', envir = RCy3env)

RCy3.R

setDefaultBaseUrl <- function(defbaseurl='http://localhost:1234/v1'){
    assign(".defaultBaseUrl", defbaseurl, envir = RCy3env)
}

*.R

Replace all base.url = .defaultBaseUrl,s to base.url = get(".defaultBaseUrl",envir = RCy3env).

AlexanderPico commented 3 years ago

Almost. You've got the assign and setDefaultBaseUrl functions right (though I'm going to remove "default" from the variable and function name, and capitalize to match conventions). But instead of replacing all the base.url params, it might be cleaner to simply remove them! They are currently in every function. We only need the base.url in a small handful of core functions (in Commands.R). i.e., the ones that actually make REST calls. So, I would insert get(".BASE_URL",envir = RCy3env) into just those few functions.

That should work, right?

AlexanderPico commented 3 years ago

Note that there is some strong advice against using global variables: https://stackoverflow.com/questions/12598242/global-variables-in-packages-in-r. In this advice, they actually say that it is better to do it the way we are currently doing it, by passing variables around in every function.

So, I'm not totally convinced yet that my latest idea is a good one...

In practice, let's imagine a workflow script that uses 12 functions from RCy3. Currently, if the end-user wants to use specify a base.url, they might define my.base.url=*** at the top of their script and then they have to be aware to include base.url=my.base.url as a param in each of the 12 functions in their script.

In the proposed approach, they would define setBaseUrl=*** once at the top of their script and then it would just work.

HOWEVER, if they run another script in that same R session, then it will also be using this new base.url. It stays until they either reset it or they reset their R environment or reload the RCy3 package. I think this is where the advice against this approach is centered: it can lead to unexpected behaviors that are difficult to troubleshoot.

So, what I need to know from end-users who have to specify an alt base.url is which would be more painful for you? (1) Explicitly setting base.url in each function, where it's clear and easy to troubleshoot, or (2) Setting it once in your scripts and having to be aware of the value of that hidden variable at all times.

In real world use cases, do you simply always want it to be an alternate value (like using a special port for CyREST) or do you need to change it for various scripts within an R session?

m-grimes commented 3 years ago

Alex

For what it’s worth, we don’t mess with the base URL in any way right now. I would rather not have to set it up each time.

=$0.02

Mark

University of Montana

On 8 Mar 2021, at 13:26, Alexander Pico wrote:

Note that there is some strong advice against using global variables: https://stackoverflow.com/questions/12598242/global-variables-in-packages-in-r. In this advice, they actually say that it is better to do it the way we are currently doing it, by passing variables around in every function.

So, I'm not totally convinced yet that my latest idea is a good one...

In practice, let's imagine a workflow script that uses 12 functions from RCy3. Currently, if the end-user wants to use specify a base.url, they might define my.base.url=*** at the top of their script and then they have to be aware to include base.url=my.base.url as a param in each of the 12 functions in their script.

In the proposed approach, they would define setBaseUrl=*** once at the top of their script and then it would just work.

HOWEVER, if they run another script in that same R session, then it will also be using this new base.url. It stays until they either reset it or they reset their R environment or reload the RCy3 package. I think this is where the advice against this approach is centered: it can lead to unexpected behaviors that are difficult to troubleshoot.

So, what I need to know from end-users who have to specify an alt base.url is which would be more painful for you? (1) Explicitly setting base.url in each function, where it's clear and easy to troubleshoot, or (2) Setting it once in your scripts and having to be aware of the value of that hidden variable at all times.

In real world use cases, do you simply always want it to be an alternate value (like using a special port for CyREST) or do you need to change it for various scripts within an R session?

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/cytoscape/RCy3/issues/69#issuecomment-793051781

AlexanderPico commented 3 years ago

Thanks Mark. This change would only affect end-users who have to specify an alternative base.url. For you and 99% of other users, there would be no change regardless of the solution we go with here.

OmarAshkar commented 3 years ago

@kozo2 @AlexanderPico How do I connect RCy3 on bioc image container to local cytoscape? my cytoscapePing(base.url = "127.0.0.1:8787") gives me Error in .cyFinally(res) :

AlexanderPico commented 3 years ago

@OmarAshkar Sorry I didn't see this comment. Please open new issues in the future to get better attention.

Regarding your use case, I'm not familiar enough with bioc image containers. Assuming is something like running a Docker container on a server with RCy3 in it and wanting it to communicate with a local Cytoscape, well that is very tricky indeed! Setting your base.url will NOT work. The base.url is to tell CyREST how to connect to Cytoscape.

However, we are also interested in support this cloud-local scenario and have a ton of work implementing a Jupyter-Bridge technology. This will allow Jyputer-hosted scripts running RCy3 to work with local Cytoscape instances. This will be available in the next major release of RCy3 (late May 2021).