Skip to content

[_795] allow options to be accessed as attrs of metadata obj #796

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.

Sign up for GitHub

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

Open
d-w-moore wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[_795] allow options to be accessed as attrs of metadata obj #796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@ Disabling AVU reloads from the iRODS server

With the default setting of `reload = True`, an `iRODSMetaCollection` will
proactively read all current AVUs back from the iRODS server after any
metadata write done by the client. This helps methods such as `items()`
metadata write done by the client. This helps methods such as `keys()` and `items()`
to return an up-to-date result. Setting `reload = False` can, however, greatly
increase code efficiency if for example a lot of AVUs must be added or deleted
at once without reading any back again.
Expand All @@ -952,6 +952,22 @@ current_metadata = obj.metadata().items()
print(f"{current_metadata = }")
```

By way of explanation, please note that calls of the form
`obj.metadata([opt1=value1[,opt2=value2...]])` will always
produce new `iRODSMetaCollection` objects - which nevertheless share the same
session object as the original, as the copy is shallow in most respects.
This avoids always mutating the current instance and thus prevents any need to
implement context manager semantics when temporarily altering options such
as `reload` and `admin`.

Additionally note that the call `obj.metadata()` without option parameters
always syncs the AVU list within the resulting `iRODSMetaCollection` object to
what is currently in the catalog, because the original object is unmutated with
respect to all options (meaning `obj.metadata.reload` is always `True`) -- that
is, absent any low-level meddling within reserved fields by the application.
Thus, `obj.metadata().items()` will always agree with the in-catalog AVU list
whereas `obj.metadata.items()` might not.

Subclassing `iRODSMeta`
---------------------
The keyword option `iRODSMeta_type` can be used to set up any `iRODSMeta`
Expand Down
13 changes: 8 additions & 5 deletions irods/manager/metadata_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,17 @@
pass


_default_MetadataManager_opts = {

Check failure on line 30 in irods/manager/metadata_manager.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff N816 

N816: Variable `_default_MetadataManager_opts` in global scope should not be mixedCase [pep8-naming:mixed-case-variable-in-global-scope]
'admin':False,
'timestamps':False,
'iRODSMeta_type':iRODSMeta,
'reload':True
}

Check failure on line 35 in irods/manager/metadata_manager.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting
Comment on lines +30 to +35
Copy link
Contributor

@alanking alanking Feb 9, 2026

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

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

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

Copy link
Contributor

@alanking alanking Feb 10, 2026

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

@alanking alanking Feb 10, 2026

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

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

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.

Copy link
Contributor

@alanking alanking Feb 10, 2026

Choose a reason for hiding this comment

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

Sounds good.


class MetadataManager(Manager):

def __init__(self, *_):
self._opts = {
'admin':False,
'timestamps':False,
'iRODSMeta_type':iRODSMeta
}
self._opts = _default_MetadataManager_opts.copy()
super().__init__(*_)

@property
Expand Down
6 changes: 6 additions & 0 deletions irods/meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@


class iRODSMetaCollection:
def __getattr__(self,n):

Check failure on line 134 in irods/meta.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff D105 

D105: Missing docstring in magic method [pydocstyle:undocumented-magic-method]
Copy link
Contributor

@alanking alanking Feb 9, 2026

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.

from irods.manager.metadata_manager import _default_MetadataManager_opts

Check failure on line 135 in irods/meta.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting
if n in _default_MetadataManager_opts:
return self._manager._opts[n]

Check failure on line 137 in irods/meta.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff SLF001 

SLF001: Private member accessed: `_opts` [flake8-self:private-member-access]
raise AttributeError
Comment on lines +136 to +138
Copy link
Contributor

@alanking alanking Feb 9, 2026

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

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

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

Copy link
Contributor

@alanking alanking Feb 10, 2026

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

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

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

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

(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 •
edited
Loading

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

@alanking alanking Feb 10, 2026

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 •
edited
Loading

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.


def __call__(self, **opts):
"""
Optional parameters in **opts are:
Expand Down
16 changes: 8 additions & 8 deletions irods/test/meta_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,24 +820,24 @@
def test_cascading_changes_of_metadata_manager_options__issue_709(self):
d = None

def get_option(metacoll, key):
return metacoll._manager._opts[key]
# def get_option(metacoll, key):
# return metacoll._manager._opts[key]

Check failure on line 824 in irods/test/meta_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-check

Ruff ERA001 

ERA001: Found commented-out code [eradicate:commented-out-code]

Check failure on line 824 in irods/test/meta_test.py

View workflow job for this annotation

GitHub Actions / ruff-lint / ruff-format

Ruff format 

Improper formatting
try:
d = self.sess.data_objects.create(f'{self.coll.path}/issue_709_test_1')
m = d.metadata
self.assertEqual(get_option(m, 'admin'), False)
self.assertEqual(m.admin, False)

m2 = m(admin=True)
self.assertEqual(get_option(m2, 'timestamps'), False)
self.assertEqual(get_option(m2, 'admin'), True)
self.assertEqual(m2.timestamps, False)
self.assertEqual(m2.admin, True)

m3 = m2(timestamps=True)
self.assertEqual(get_option(m3, 'timestamps'), True)
self.assertEqual(get_option(m3, 'admin'), True)
self.assertEqual(m3.timestamps, True)
self.assertEqual(m3.admin, True)
self.assertEqual(m3._manager.get_api_keywords().get(kw.ADMIN_KW), "")

m4 = m3(admin=False)
self.assertEqual(get_option(m4, 'admin'), False)
self.assertEqual(m4.admin, False)
self.assertEqual(m4._manager.get_api_keywords().get(kw.ADMIN_KW), None)
finally:
if d:
Expand Down
Loading