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

Fix for get_latest_source_version being called unnecessarily. #237

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DnlRKorn
Copy link
Contributor

Made pretty much all calls for source versioning into dynamic calls.

Resolves issue #236 and number 3 of issue #227 .

Modified the DataSource class in kgxmodel.py to make source_version generated when needed instead of at objection creation time. Modified build_manager.py to get strings which describe graph_version as needed for building the graph and dependant subgraphs and not unnecessarily.

Daniel Korn and others added 4 commits July 23, 2024 17:39
…enerated when needed instead of at objection creation time. Modified build_manager.py to get strings which describe graph_version as needed for building the graph and dependant subgraphs and not unnecessarily.
@EvanDietzMorris
Copy link
Contributor

Finally got to testing this out. The subgraph logic was broken here because after your changes it would always try to dynamically generate the version for a subgraph using information from the graph spec, but we had and still want the ability to hard code a subgraph version in the graph spec, ignoring latest versions or other stuff in the graph spec. I made some changes to handle that, and generally improved the error catching and messaging around specifying subgraph versions.

I made some other (possibly questionable) changes to the DataSource part, entirely because the way you did it forced us to always assign a function to get_source_version, even when a version was explicitly specified in the Spec, and that sort of felt bad to me. I wanted us to still be able to provide a string value for the version without using a lambda function when we wanted to. The changes I added accomplish that, but honestly might have made it all a bit messier and harder to understand. Take a look and see what you think and we can change it back or to something else if you want.

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.

2 participants