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.

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

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

Closed
kozo2 opened this issue Sep 21, 2019 · 19 comments
Closed

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

kozo2 opened this issue Sep 21, 2019 · 19 comments
Assignees
Labels
question Further information is requested
Milestone

Comments

@kozo2
Copy link
Member

kozo2 commented Sep 21, 2019

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?

@kozo2 kozo2 added enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Sep 21, 2019
@AlexanderPico
Copy link
Member

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
Copy link
Member

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

@AlexanderPico AlexanderPico self-assigned this Sep 24, 2019
@AlexanderPico AlexanderPico removed help wanted Extra attention is needed question Further information is requested labels Sep 24, 2019
@AlexanderPico AlexanderPico added this to the 2.5.x milestone Sep 24, 2019
@AlexanderPico
Copy link
Member

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"

@AlexanderPico AlexanderPico added question Further information is requested and removed enhancement New feature or request labels Sep 24, 2019
@kozo2
Copy link
Member Author

kozo2 commented Sep 28, 2019

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 kozo2 reopened this Sep 28, 2019
@kozo2 kozo2 changed the title How can I replace .defaultBaseUrl ? Do we need to add setDefaultBaseUrl function? Sep 28, 2019
@kozo2
Copy link
Member Author

kozo2 commented Sep 28, 2019

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

@kozo2 kozo2 changed the title Do we need to add setDefaultBaseUrl function? Do we need to add "setDefaultBaseUrl" function to RCy3? Sep 28, 2019
@AlexanderPico
Copy link
Member

AlexanderPico commented Sep 28, 2019

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
Copy link
Member

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
Copy link

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

@kozo2
Copy link
Member Author

kozo2 commented Dec 25, 2020

@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
Copy link

yangli-ai commented Dec 26, 2020

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
Copy link
Member Author

kozo2 commented Dec 27, 2020

@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 AlexanderPico reopened this Mar 7, 2021
@AlexanderPico
Copy link
Member

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
Copy link
Member Author

kozo2 commented Mar 8, 2021

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

m-grimes commented Mar 8, 2021 via email

@AlexanderPico
Copy link
Member

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
Copy link

@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
Copy link
Member

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants