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

schema/context: restore some backlinks support #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bradh352
Copy link
Contributor

@bradh352 bradh352 commented Feb 12, 2025

In libyang v1 the schema nodes had a backlinks member to be able to look up dependents of the node. SONiC depends on this to provide functionality it uses and it needs to be exposed via the python module.

In theory, exposing the 'dfs' functions could make this work, but it would likely be cost prohibitive since walking the tree would be expensive to create a python node for evaluation in native python.

Instead this PR depends on the this libyang PR: CESNET/libyang#2351
And adds thin wrappers.

This implementation provides 2 python functions:

  • Context.find_backlinks_paths() - This function can take the path of the base node and find all dependents. If no path is specified, then it will return all nodes that contain a leafref reference.
  • Context.find_leafref_path_target_paths() - This function takes an xpath, then returns all target nodes the xpath may reference. Typically only one will be returned, but multiples may be in the case of a union.

A user can build a cache by combining Context.find_backlinks_paths() with no path set and building a reverse table using
Context.find_leafref_path_target_paths()

@bradh352
Copy link
Contributor Author

I'd like to see something similar to this merged so we don't have to maintain a patchset. I'm willing to make changes as we do not have this API solidified yet.

@bradh352 bradh352 force-pushed the backlinks branch 3 times, most recently from 5349d57 to 14ad790 Compare February 12, 2025 01:58
@rjarry
Copy link
Collaborator

rjarry commented Feb 12, 2025

Hi,

I don't like having custom code not supported by libyang in the python bindings unless it is specifically related to the adaptation to python or if it is really trivial. In this case, it is adding back support for a removed feature from libyang v2 and it involves fairly complex functions.

Could you add the feature to https://github.com/CESNET/libyang instead? And then refer to that API from the python bindings?

Thanks!

@bradh352
Copy link
Contributor Author

Hi,

I don't like having custom code not supported by libyang in the python bindings unless it is specifically related to the adaptation to python or if it is really trivial. In this case, it is adding back support for a removed feature from libyang v2 and it involves fairly complex functions.

Could you add the feature to https://github.com/CESNET/libyang instead? And then refer to that API from the python bindings?

Thanks!

Sure I can do that, I just wasn't sure how well received it would be.

That said, to make sure we're on the same page and we don't have too many back and forths on this topic, can you suggest a proposed set of public function prototypes that would be acceptable to be merged into libyang? I'll do the heavy backend lifting.

For instance, I'd assume instead of returning the paths as strings, you'd probably require the libyang api to return an LY_ARRAY of lysc_nodes, as I don't see any public functions that return an array of string paths.

@rjarry
Copy link
Collaborator

rjarry commented Feb 12, 2025

I'm not sure what is the kind of public API most likely to be accepted. I'm not actively involved in the development of libyang anymore. But feel free to drop them an RFC patch asking for their opinion. They are pretty open to changes in general.

Cheers!

bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 12, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 12, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 12, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 12, 2025
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (1afc2a6) to head (66c7e27).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
libyang/context.py 83.33% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   80.19%   80.63%   +0.43%     
==========================================
  Files           9       10       +1     
  Lines        3444     3708     +264     
==========================================
+ Hits         2762     2990     +228     
- Misses        682      718      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 13, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 13, 2025
@bradh352
Copy link
Contributor Author

@michalvasko can you comment on a public API that might be accepted for libyang itself that I can then wrap for libyang-python ? I can submit patches to both libyang and a follow-up to libyang-python. I want to solidify the API since we're moving forrward with porting SONiC to libyang3.

@bradh352
Copy link
Contributor Author

@rjarry I've done your suggestion. PR here for libyang: CESNET/libyang#2351

Now this PR is just a thin wrapper around that new functionality.

@michalvasko please provide feedback on this implementation. Willing to make changes, but want to come to a consensus fairly early so what we apply in SONiC isn't too far off from what is accepted upstream here.

bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 16, 2025
@bradh352
Copy link
Contributor Author

This PR has been updated to use the replacement PR CESNET/libyang#2352 which was derived from the original. Rework PR to be compatible.

In libyang v1 the schema nodes had a backlinks member to be able to
look up dependents of the node.  SONiC depends on this to provide
functionality it uses and it needs to be exposed via the python
module.

In theory, exposing the 'dfs' functions could make this work, but
it would likely be cost prohibitive since walking the tree would
be expensive to create a python node for evaluation in native
python.

Instead this PR depends on the this libyang PR:
CESNET/libyang#2352
And adds thin wrappers.

This implementation provides 2 python functions:
 * Context.find_backlinks_paths() - This function can
   take the path of the base node and find all dependents.  If
   no path is specified, then it will return all nodes that contain
   a leafref reference.
 * Context.find_leafref_path_target_paths() - This function takes
   an xpath, then returns all target nodes the xpath may reference.
   Typically only one will be returned, but multiples may be in the
   case of a union.

A user can build a cache by combining Context.find_backlinks_paths()
with no path set and building a reverse table using
Context.find_leafref_path_target_paths()

Signed-off-by: Brad House <[email protected]>
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 17, 2025
@bradh352
Copy link
Contributor Author

@rjarry looks like @michalvasko took my PR to libyang and massaged it into an acceptable state. Can you tell me if if this modification to libyang-python looks acceptable? Or are there any changes you'd need?

bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 20, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 20, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 21, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 25, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Feb 28, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 1, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 1, 2025
bradh352 added a commit to bradh352/sonic-buildimage that referenced this pull request Mar 3, 2025
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

Successfully merging this pull request may close these issues.

3 participants