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

ENH: add search term for retrieving Entries that are nested under an ancestor #108

Conversation

shilorigins
Copy link
Collaborator

@shilorigins shilorigins commented Nov 21, 2024

Description

Add support for ancestor SearchTerm to FilestoreBackend.search
Add FilestoreBackend._gather_progeny utility method

Closes #102

Motivation and Context

With an Entry in one Nestable, it is impossible to retrieve the corresponding Entry in another Nestable using the existing SearchTerms. The attributes of the known Entry do not reliably correspond to attributes of the desired Entry, other than the .pv_name or .title, so it is necessary to be able to limit searches to Entrys that are nested under a known Nestable.

A concrete use case is comparing snapshots. For each PV in the first snapshot, the corresponding PV must be found in the second snapshot using only the PV string. This becomes possible using the new search term.

The current implementation uses a utility method to gather all Entrys nested under an ancestor Entry. This enables caching the result using functools.cache, which lets us avoid:

  1. Traversing the ancestor's DAG for each entry we check
  2. Iterating over the search terms in advance in order to gather all progeny before checking the search conditions

Calling cache directly rather than as a decorator limits the caching to each FilestoreBackend.search call, so changes to the data between searches are reflected properly. I believe each cache should be garbage collected after each search, but I haven't been able to verify this yet.

How Has This Been Tested?

A unit test in tests/test_backend and an integration test in tests/test_client.

Where Has This Been Documented?

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@shilorigins shilorigins added enhancement New feature or request Core Client Core functionality (Python Client) related issues labels Nov 21, 2024
@shilorigins shilorigins added this to the Minimum Viable Product milestone Nov 21, 2024
@shilorigins shilorigins self-assigned this Nov 21, 2024
@shilorigins shilorigins force-pushed the devagr/CSWOPS-66-ancestor-search-option branch from c8fe14d to 452291f Compare November 21, 2024 22:57
@shilorigins shilorigins force-pushed the devagr/CSWOPS-66-ancestor-search-option branch from 452291f to 7a3a1f9 Compare November 25, 2024 23:36
The new term uses a utility method FilestoreBackend._gather_reachable to
check whether each entry matches the term.  This method collects all
Entries reachable from the ancestor, and the result is cached for the
duration of the search.
.save_entry didn't flatten the new entry when inserting it into the
backend cache, and the cache is reconstructed from _root in every
_load_or_initialize call.  This means once the filestore_backend
fixture is set up, the final entry's children aren't accessible from
the cache until _load_or_initialize is called.

Flattening new entries in .save_entry fixes this issue.
@shilorigins shilorigins force-pushed the devagr/CSWOPS-66-ancestor-search-option branch from 7a3a1f9 to 500521b Compare November 25, 2024 23:41
@shilorigins shilorigins marked this pull request as ready for review November 26, 2024 01:01
tangkong
tangkong previously approved these changes Nov 26, 2024
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 I left suggestions for a slightly expanded test and a more permissive type hint, but I think the core of this is hard to disagree with. I imagine this gets refactored a bit with the Visitor concept

superscore/backends/filestore.py Outdated Show resolved Hide resolved
superscore/tests/test_client.py Show resolved Hide resolved
superscore/tests/test_backend.py Show resolved Hide resolved
superscore/backends/filestore.py Outdated Show resolved Hide resolved
superscore/tests/test_backend.py Show resolved Hide resolved
@shilorigins shilorigins merged commit 3c3bc18 into pcdshub:master Dec 3, 2024
11 checks passed
@shilorigins shilorigins deleted the devagr/CSWOPS-66-ancestor-search-option branch December 3, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Client Core functionality (Python Client) related issues enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: add ability to limit Entry searches bases on ancestor
2 participants