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

Unexpected error when __sizeof__ is malformed when running Python #13491

Open
4 tasks done
dfalbel opened this issue Aug 14, 2023 · 12 comments
Open
4 tasks done

Unexpected error when __sizeof__ is malformed when running Python #13491

dfalbel opened this issue Aug 14, 2023 · 12 comments

Comments

@dfalbel
Copy link
Member

dfalbel commented Aug 14, 2023

System details

RStudio Edition : Desktop
RStudio Version : Version 2023.09.0-daily+303
OS Version      : MacOS Ventura 13.5
R Version       : 4.3.1

Steps to reproduce the problem

Execute the folling in the reticulate repl:

class Obj:
  def __init__ (self):
    self.x = 1
  
  def __sizeof__(self):
    return 1/0

obj = Obj()

Once obj is created we get an error:

── Python Exception Message ──────────────────────────────────────────────────────────────────────────────────────────────────
Traceback (most recent call last):
  File "<string>", line 6, in __sizeof__
ZeroDivisionError: division by zero

── R Traceback ───────────────────────────────────────────────────────────────────────────────────────────────────────────────
     ▆
  1. ├─reticulate::repl_python()
  2. │ ├─base::tryCatch(repl(), interrupt = identity)
  3. │ │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
  4. │ │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
  5. │ │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
  6. │ └─reticulate (local) repl()
  7. │   └─reticulate:::readline(prompt = prompt)
  8. └─.rs.reticulate.detectChanges("__main__")
  9.   ├─changedObjects$append(.rs.reticulate.describeObject(var, newObjects))
 10.   └─.rs.reticulate.describeObject(var, newObjects)
 11.     └─sys$getsizeof(object)
 12.       └─reticulate:::py_call_impl(callable, call_args$unnamed, call_args$named)

Describe the problem in detail

Once obj is defined, the IDE is triggered to call .rs.reticulate.describeObject which tries to obtain the object size via:

# get object size
sys <- reticulate::import("sys")
size <- sys$getsizeof(object)

sys.getsizeof calls the __sizeof__ that can raise errors.

Describe the behavior you expected

Unless the user explicitly calls sys.getsizeof() we shouldn't expose the error to the user.

  • I have read the guide for submitting good bug reports.
  • I have installed the latest version of RStudio, and confirmed that the issue still persists.
  • If I am reporting an RStudio crash, I have included a diagnostics report.
  • I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.
@dfalbel dfalbel added bug new New incoming issues, ready for initial review. labels Aug 14, 2023
@ronblum ronblum added python and removed new New incoming issues, ready for initial review. labels Aug 18, 2023
@t-kalinowski
Copy link
Member

Related: #11543

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs, per https://github.com/rstudio/rstudio/wiki/Issue-Grooming. Thank you for your contributions.

@github-actions github-actions bot added the stale Issues which have been closed automatically due to inactivitiy. label Feb 20, 2024
Copy link

github-actions bot commented Mar 5, 2024

This issue has been automatically closed due to inactivity.

@github-actions github-actions bot closed this as completed Mar 5, 2024
@lucaverteramo
Copy link

I believe the issue is still not solved (bumping it for the bot)

@kevinushey kevinushey removed the stale Issues which have been closed automatically due to inactivitiy. label Apr 11, 2024
@kevinushey kevinushey reopened this Apr 11, 2024
@kevinushey
Copy link
Contributor

Any thoughts on what the right solution here is? Do we need to catch errors when invoking sizeof and return 0 or NA on error, or something similar?

Or is there a way to get the "internal" size of an object, bypassing any dunder methods defined for the class?

@t-kalinowski
Copy link
Member

I don't know of another way.

What do we use size for? Would it be a big loss to blanket return NA for Python object sizes?

The only other way I think of is to setup error handlers:

last_error <- reticulate::py_last_error()
size <- tryCatch(
          sys$getsizeof(object), 
          python.builtin.BaseException = function(e) {
            reticulate::py_last_error(last_error) # restore previous exception
            NA_integer_
         })

@kevinushey
Copy link
Contributor

I think the size here is just for reporting a size in the Environment pane, so it's probably fine for that to be unknown / NA if we couldn't compute it. (Reporting the object size in Python is perilous anyhow, given that objects can be shared...)

@lucaverteramo
Copy link

Could it be related to a bug in the AnnData Python package instead? If the unexpected behaviour is only present when subsetting using this package (see #1332

@t-kalinowski
Copy link
Member

t-kalinowski commented Apr 12, 2024

In that case, if the bug is so rare it only applies to AnnData in practice, then perhaps the fix is to advise users to call:

reticulate:::py_register_load_hook("anndata", function() {
  reticulate::py_run_string(local = TRUE, r"---(
    import anndata
    anndata.AnnData.__sizeof__ = lambda: -2147483648 # R NA_integer_
  )---")
})

( and suggest a PR to AnnData )

@kevinushey
Copy link
Contributor

I think I agree with that. I imagine the implicit contract in __sizeof__ is that it would return a number, or perhaps some sentinel value for "unknown" sizes (like None?). Either way, I don't think it should throw an exception.

@jthomasmock
Copy link
Contributor

@t-kalinowski do you mind commenting on their repo given your knowledge of the problem space here?

@t-kalinowski
Copy link
Member

t-kalinowski commented Apr 18, 2024

@jthomasmock, It looks like this has already been fixed upstream: rstudio/reticulate#1332 (comment)
( Reported first in scverse/anndata#1127, maybe fixed with scverse/anndata#1230 ?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants