Skip to content

Generate example epi_archive on demand. #125

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

Closed
dajmcdon opened this issue Jun 27, 2022 · 5 comments
Closed

Generate example epi_archive on demand. #125

dajmcdon opened this issue Jun 27, 2022 · 5 comments
Assignees

Comments

@dajmcdon
Copy link
Contributor

archive_cases_dv_subset needs to be regenerated or reworked to form an epi_archive from more basic parts on demand; it currently appears to load with the old class definition, missing the extra "\n", plus some bad srcrefs that may make user debugging of tweaked examples harder:

archive_cases_dv_subset$print
<srcref: file "C:/Users/chloe/Desktop/Summer2022/epiprocess/R/archive.R" chars 162:19 to 187:11>
<environment: 0x55c5b02d6358>
Warning message:
In getSrcLines(srcfile, x[7L], x[8L]) :
  restarting interrupted promise evaluation
> body(archive_cases_dv_subset$print)
{
    cat("An `epi_archive` object, with metadata:\n")
    cat(sprintf("* %-9s = %s\n", "geo_type", self$geo_type))
    cat(sprintf("* %-9s = %s\n", "time_type", self$time_type))
    if (!is.null(self$additional_metadata)) {
        sapply(self$additional_metadata, function(m) {
            cat(sprintf("* %-9s = %s\n", names(m), m))
        })
    }
    cat("----------\n")
    cat(sprintf("* %-14s = %s\n", "min time value", min(self$DT$time_value)))
    cat(sprintf("* %-14s = %s\n", "max time value", max(self$DT$time_value)))
    cat(sprintf("* %-14s = %s\n", "min version", min(self$DT$version)))
    cat(sprintf("* %-14s = %s\n", "max version", max(self$DT$version)))
    cat("----------\n")
    cat(sprintf("Data archive (stored in DT field): %i x %i\n", 
        nrow(self$DT), ncol(self$DT)))
    cat("----------\n")
    cat(sprintf("Public methods: %s", paste(names(epi_archive$public_methods), 
        collapse = ", ")))
}

The quick fix is to just regenerate the object + file an Issue to later figure out how to generate the example epi_archive on demand or otherwise ensure that it is current with the implementation of epi_archive.

@dajmcdon
Copy link
Contributor Author

Originally described in #85

@brookslogan
Copy link
Contributor

Copying from #85: there is a fix in 8f36d44 that can be applied. This should naturally occur as a merge conflict resolution of #101 with #85 (or whichever PR/commit revises the sample archive's name).

@brookslogan
Copy link
Contributor

brookslogan commented Jul 1, 2022

The "fix" above seemed to work and avoid the need for @include tags when I tested it off of 8f36d44, as it seemed to allow me to load or document the package without errors. However, now, when I already have epiprocess loaded via devtools and I modify the epi_archive class to try to assign to new fields I've forgotten to initially set to NULL, I get complaints that I "cannot add bindings to a locked environment", even after fixing the problem [and even though the earlier buggy package version's load seemed like it failed midway]. I have to unloadNamespace("epiprocess") first to have it successfully reload. Somehow, it seems to successfully use the new epi_archive if loading for the first time, but the old epi_archive if reloading.

Maybe my statement about not needing @includes is wrong [although adding @includes and the generated(-on-successful-document) Collate: directive don't seem to help]. Maybe my method of applying delayedAssign is wrong. Maybe there's a devtools[/upstream] bug. [It seems like maybe (part of) the issue is that the buggy package is allowed to be loaded, since this is a delayedAssign. However, document seems to involve multiple loads of the current version of the package, which triggers the error if the current version is the buggy version, giving the impression that the buggy version would not have been loaded/reloaded at all, when (if moving from nonbuggy to buggy) the first load in document would have gone through. The trigger for the error seems to be that reloading causes pkgload::unregister to be called, which called pkgload:::unregister_namespace, which forces everything in the (old) environment for some reason, which, if the old loaded version contains a error-raising-on-eval promise, will trigger that error. This is clearer if not trying to recover, as on later attempted reloads, one will get

In eapply(ns_env(name), force, all.names = TRUE) :
  restarting interrupted promise evaluation

Link to the comments on the reason. Added in rlib/pkgload#157.
]

@brookslogan
Copy link
Contributor

brookslogan commented Jul 2, 2022

To summarize the actual issue here:

  1. Start from a non-buggy package.
  2. Introduce a bug that allows the package to load, but causes a promise in the package namespace to produce an error when evaluated.
  3. load_all() the package. It will successfully load.
  4. load_all() the package again. It will emit an error message about the bug.
  5. load_all() the package again. It will emit an error message about the bug + a warning about restarting interrupted promise evaluation.
  6. Any further load_all()s will produce the same result as in 5. Fixing the bug has no effect on the results after step 3 has been performed.
  7. To escape the cycle without restarting, unloadNamespace("pkgname") seems to work, although I'm not sure whether it's the "proper" function to call here.

Note that document() appears to load_all() twice, so it would do both steps 3&4 together.

This could be pretty surprising while developing. Maybe we can add a .onLoad function that tests creation of a 1-row archive to help prevent it, or make the archive promise into a regular object. [this does not actually prevent loading / future unloading of the package, although it might change the specific error sequence above. Another potential approach --- only if unregister calls .onUnload [it doesn't]: try to use .onUnload to do the eapply step before pkgload does it (ideally only if we're going to hit that eapply step anyway), but with a more informative error message if something goes wrong, OR to wrap the code of any promises with something that will produce more informative error messages. Another approach would be make a delayedAssign substitute that will check when evaluated if it's in the middle of an unregister/unregister_namespace and wrap errors accordingly.]


A separate issue with the current implementation is that the memory use is going to be about double what it needs to be, as both archive_cases_dv_dt and archive_cases_dv will be evaluated and stick around. There's probably a way around this, but it raises yet another issue which is that when memory actually matters (it doesn't matter that much for these small examples), the user may want the ability to "move" x into the archive / share x with the archive, not cloning.

@brookslogan
Copy link
Contributor

Resolved with #101.

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

No branches or pull requests

2 participants