Skip to content

Conversation

@d-w-moore
Copy link
Collaborator

No description provided.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address or explicitly ignore Ruff as you see fit.

Comment on lines +136 to +138
if n in _default_MetadataManager_opts:
return self._manager._opts[n]
raise AttributeError
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of _default_MetadataManager_opts is a little confusing to me. Does this mean that we cannot access non-default options?

Does something like this improve the situation, or make it worse?

Suggested change
if n in _default_MetadataManager_opts:
return self._manager._opts[n]
raise AttributeError
if (attr := self._manager._opts.get(n)) is None:
raise AttributeError(f"Attribute [{n}] not found")
return attr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it that way and got a never-ending recursion, unfortunately : (

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if it has to do with the fact that iRODSMeta is the iRODSMeta_type for _opts...

Does this work?

Suggested change
if n in _default_MetadataManager_opts:
return self._manager._opts[n]
raise AttributeError
if n in self._manager._opts:
return self._manager._opts[n]
raise AttributeError

I'm mostly trying to see if we can avoid having to use _default_MetadataManager_opts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's how I originally tried it. Or, similar. When these infinite loopings happen, it's a pain to figure out why. I'll give it another go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(py3) daniel@yuggoth:~/python-irodsclient$ python try.py 
Traceback (most recent call last):
  File "/home/daniel/python-irodsclient/try.py", line 4, in <module>
    d.metadata(admin=True).admin
    ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniel/python-irodsclient/irods/meta.py", line 151, in __call__
    x = copy.copy(self)
        ^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/copy.py", line 97, in copy
    return _reconstruct(x, None, *rv)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/copy.py", line 260, in _reconstruct
    if hasattr(y, '__setstate__'):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/daniel/python-irodsclient/irods/meta.py", line 139, in __getattr__
    if n in self._manager._opts:
            ^^^^^^^^^^^^^
  File "/home/daniel/python-irodsclient/irods/meta.py", line 139, in __getattr__
    if n in self._manager._opts:
            ^^^^^^^^^^^^^
  File "/home/daniel/python-irodsclient/irods/meta.py", line 139, in __getattr__
    if n in self._manager._opts:
            ^^^^^^^^^^^^^
  [Previous line repeated 993 more times]
RecursionError: maximum recursion depth exceeded

Copy link
Collaborator Author

@d-w-moore d-w-moore Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above dump is the result. That's why I changed the code to be what it currently is....

I don't know why the one self._manager reference is ok but the other isn't.

It's a mystery....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that stinks. Okay, please leave a comment regarding this situation above the usage of _default_MetadataManager_opts here and above the definition of _default_MetadataManager_opts in metadata_manager.py so that we understand why it is being done this way.

Copy link
Collaborator Author

@d-w-moore d-w-moore Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem appears to be that the new object m has not had _opts set on it yet. Still, why it's a problem when using in but not in the return statement is strange.



class iRODSMetaCollection:
def __getattr__(self,n):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing n to name or some other more descriptive parameter name.

Comment on lines +30 to +35
_default_MetadataManager_opts = {
'admin':False,
'timestamps':False,
'iRODSMeta_type':iRODSMeta,
'reload':True
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this buy us over just... adding the new option to self._opts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow doing it this way avoided a bottomless recursion. I do not know why....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave a comment above this explaining why it is necessary, and we can resolve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you said you don't know why, but at least describe what we are trying to prevent by doing it this way even if we don't know why the situation is happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.
Found a link that might provide insight.
https://nedbatchelder.com/blog/201010/surprising_getattr_recursion
I'm still going to try tracking it down because the copy.copy() has already happened in this case, so it... shouldn't be happening but is.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants