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

Dependency loading timing #179

Closed
eywalker opened this issue Dec 16, 2015 · 7 comments
Closed

Dependency loading timing #179

eywalker opened this issue Dec 16, 2015 · 7 comments

Comments

@eywalker
Copy link
Contributor

Currently, dependencies among tables are loaded only upon call to the populate method. Otherwise, we must first call load. This is clearly inconvenient, and we should think of a good strategy to load all the dependencies, but hopefully only as necessary.

@dimitri-yatsenko
Copy link
Member

Loading dependencies is a somewhat expensive operation. In pull request #176, dependencies are loaded once at the start of each high-level user-invoked function that requires dependencies: populate, progress, erd, etc.

@eywalker
Copy link
Contributor Author

Hmm based on the modified files in #176, I did not see load getting called from anywhere but populate...

On Dec 16, 2015, at 2:25 PM, Dimitri Yatsenko [email protected] wrote:

Loading dependencies is a somewhat expensive operation. In pull request #176, dependencies are loaded once at the start of each high-level user-invoked function that requires dependencies: populate, progress, erd, etc.


Reply to this email directly or view it on GitHub.

@eywalker
Copy link
Contributor Author

Ah, I see that it's getting called from within populated_from. However, this doesn't address the issue of needing to load when performing other operations like deletion and erd, as well as any look ups like parents and reference. I suggest that we implement load to take on a flag like force. When the force is set to False, it will check whether loading has ever been performed, and if not, perform the loading of dependencies, but not otherwise. We can pass force=True at places where it's critical to have correct dependency map (e.g. populated_from and delete). This was in fact how loading of dependency used to be implemented in the past, and we even had finer granularity for loading.

@dimitri-yatsenko
Copy link
Member

Dependencies are needed for these operations:

  • BaseRelation.drop
  • BaseRelation.delete
  • the default AutoPopulate.populated_from
  • Connection.erd

The dependencies are loaded once at the start of each of these operations.

The problem with force is that it should not be an all-or-nothing deal. New dependencies may need to be loaded as new schemas are imported. So I would rather load the dependencies completely every time at the start of each of these operations.

@dimitri-yatsenko
Copy link
Member

All of these methods already load dependencies. As far as I can tell, there is no standing problem with the code. Are there any problems with with the current solution?

@eywalker
Copy link
Contributor Author

Reference to properties like parents should trigger loading, if it has not been done yet. And my intention was to use load(force=True) precisely where we want to ensure full loading, such as in drop, delete, and erd, so it's not against your suggestion.

If preferred we can use other terms, and not force, but the point is to have a variant of load that will attempt if no one has performed the loading yet.

@eywalker
Copy link
Contributor Author

And I completely failed to see that loading is already in place in methods like delete and drop as you said 😛

And now I'm remember more about thoughts that went into the design, and I'm realizing that it's simply not trivial to ensure that properties like parents and children are in sync with structure with database, aside from calling load immediately before invocation. Given this, I now agree that we can just leave the implementation as you had it in #176.

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

No branches or pull requests

2 participants