Clarify synapser's namespace

Description

This is related to as well as this PR: https://github.com/Sage-Bionetworks/synapser/pull/147

synapser uses several functions from external packages without either importing them into the synapser namespace or referencing the package with packagename::. PythonEmbedInR is imported in the NAMESPACE file, but other functions we use like methods::setGeneric() and utils:ackageVersion() are not. In the case of setGeneric(), we modify the user's global environment to ensure the function is defined there rather than calling it from the package it originates from.

This causes R CMD check to complain, and can also cause subtle bugs downstream. Specifically, if synapser is loaded but not attached by other code, then packages in Depends will also be loaded but not attached. When a package is loaded its .onLoad() function is run, which will cause a reoccurrence of because methods will not be attached. An example can be seen by running Rscript -e "requireNamespace('synapser')". The section on Imports in the R packages book (http://r-pkgs.had.co.nz/namespace.html#Imports) has more detail on this.

I'd recommend either adding importFrom(methods, setGeneric) (etc. for all functions from we use from external packages) to NAMESPACE, or prefixing each call to an external function with packagename::. The latter is generally preferable, though has a (usually negligible) performance cost.

Environment

None

Activity

Show:
Kara Woo
May 3, 2018, 10:58 PM

Ah ok I thought we were supposed to validate once the status gets changed to RESOLVED

Kenneth Daily
May 3, 2018, 11:26 PM

An idea - use the tag Meredith started - Blocked/Monitoring? And use a Jira issue/epic for a release for validators to see when it's good to validate?

Kimyen Truong
May 4, 2018, 12:42 AM

I believed that Blocked/Monitoring is meant for an issue that is being blocked. Therefore no work can be done yet until it's unblocked.

In this case, the issue has been worked on and an engineer has marked it as Resolved. While the R client validation process is similar to PLFM, SWC, and Python client, PLFM & SWC release every week. Python client users can validate the issue as soon as it's Resolved because Python user can install from the Github repository. PLFM & SWC users can validate a Resolved issue at around the week after it is Resolved, when staging stack is up. For R client, we prefer validators to validate the package the way an end users would get the package - via CRAN like repository. Historically, our R users do not validate an issue when it is changed to Resolved, but rather when we decide to release a new version, we would send out emails to ask validators to validate issues that is associated with the release version.

I am happy to see that our R users validate the issues before we even ask. This means that the current process may be out of date since Resolved does not mean the same thing as Ready-To-Be-Validated. what do you think? Currently, after releasing a version to staging-ran, I would comment on the issue with the instruction on how to validate (for some issues) and may be tag the validator to notify them.

Meredith Slota
May 4, 2018, 2:59 PM

Generally, our workflow for stack releases (any tickets in SWC or PLFM) is to have the engineer who completed the work mark it "resolved". Then, the reporter or someone who can test the work needs to validate on the staging stack once the ticket is marked "resolved", to move it to "closed" as a mechanism for letting the engineers know it's ok to release to our production stack.

For the R and Python clients, it's a bit trickier since that same process applies BUT it is a bit harder to know when the ticket is ready to be validated. I am open to ideas here on how to improve – typically, we will alert users when a block of tickets have been done and ask them to help us validate.

"Blocked" is not the right status here as it means what said – that the ticket is not ready to be worked on, either because other functionality is required first or the full set of requirements are not yet known.

Kara Woo
August 22, 2018, 5:19 PM

Looked at the DESCRIPTION and places where setGeneric() and packageVersion() were called and it looks good.

Assignee

Kimyen Truong

Reporter

Kara Woo

Labels

None

Validator

Kara Woo

Development Area

None

Release Version History

None

Fix versions

Priority

Minor
Configure