ENH: Adding and documenting configs in nx-parallel #75
ENH: Adding and documenting configs in nx-parallel #75dschult merged 22 commits intonetworkx:mainfrom
Conversation
|
Notes for reviewers(@dschult):
|
| ] | ||
|
|
||
|
|
||
| def _configure_if_nx_active(): |
There was a problem hiding this comment.
We agreed on this name in the last meeting. (ref. Notes)
|
|
||
| [tool.ruff.lint.per-file-ignores] | ||
| "__init__.py" = ['I', 'F403'] | ||
| "__init__.py" = ['I', 'F403', 'F401'] |
There was a problem hiding this comment.
'F401' is added here to not get "module imported but unused." while running pre-commit on the nx_parallel/__init__.py because we just import _configure_if_nx_active in the file to include it in the public API but we don't use it.
dschult
left a comment
There was a problem hiding this comment.
This is looking good -- I have a couple questions below. Probably tweaks in wording will clear it up. Thanks...
… joblib.parallel_config or networkx config
dschult
left a comment
There was a problem hiding this comment.
I took another look at the whole PR and the files rather than the diffs. So I've got a few more suggestions to think about. Adapt or reject them as you see fit.
Config.md
Outdated
|
|
||
| In this example, if `another_nx_backend` also internally utilizes `joblib.Parallel` (without exposing it to the user) within its implementation of the `square_clustering` algorithm, then the `nx-parallel` configurations set by `joblib.parallel_config` will influence the internal `joblib.Parallel` used by `another_nx_backend`. To prevent unexpected behavior, it is advisable to configure these settings through the NetworkX configuration system. | ||
|
|
||
| If you're using `nx-parallel` independently, either the NetworkX or `joblib` configuration system can be used, depending on your preference. |
There was a problem hiding this comment.
addressing the question of what "independently" means: one attempt
| If you're using `nx-parallel` independently, either the NetworkX or `joblib` configuration system can be used, depending on your preference. | |
| If your code using `nx-parallel` does not simultaneously use joblib (including inside another networkx backend) then either the `networkx.config` system or the `joblib.parallel_config` system can be used, depending on your preference. |
Also, I think this paragraph should appear at the top of this section, as I think most users will be in this situation.
There was a problem hiding this comment.
I don't think it would be a great idea to mention this here because there might be other conflicts too that we don't know of right now. This was just one of the cases where a conflict can occur. LMK what you think.
There was a problem hiding this comment.
I agree that we don't want to exclude other possible conflicts -- so let's not be too specific about them.
The first point I'm looking for here is a better phrase than "using nx-parallel independently". (Independently of what?)
The second point is that this seems like a nice first paragraph for this section rather than coming down here. A reader who is using nx-parallel without other joblib code will be able to read this sentence and skip the rest of this section if it doesn't apply to them.
There was a problem hiding this comment.
I hope the recent commit resolves this concern :)
There was a problem hiding this comment.
Is there any chance of trouble when the other backend doesn't use joblib?
For example, could the config using joblib.parallel_config interfere in any way with nx-cugraph or graphblas-algorithms?
I think the only way to have a "conflict" is if the other code is using joblib too. Right?
There was a problem hiding this comment.
Thank you for your question. I'm not familiar with the codebases of other backends, so I can't say for certain. However, the occurrence of the unexpected behaviours also largely depends on the specific joblib backend(loky, threading, multiprocessing, dask, etc.) that the user is employing.
Also, it's beneficial to use NetworkX's configuration when setting backend_priority and configurations within a context for multiple networkx backends, like this:
with nx.config(backend_priority = ['parallel', 'another_nx_backend`], backends=Config(parallel=<nx_parallel_configs>, another_nx_backend=<another_nx_backend_configs>)):
...There was a problem hiding this comment.
I think it doesn't matter what the codebases of the other backends are: the only config options nx-parallel uses are related to joblib. And the only way for another backend to be influenced by those choices is if they look at or use the config options in joblib. So only backends which use joblib will have a possible conflict over config values.
In the interest of putting this into place, I am going to merge this PR. And I'll open another one to make further suggestions.
| import networkx as nx | ||
| import nx_parallel as nxp | ||
|
|
||
| # enabling networkx's config for nx-parallel |
There was a problem hiding this comment.
I'm wonder if putting config issues into this section is a distraction. They are trying to understand backend usage, not how to configure. Maybe a comment line pointing to the config docs here is sufficient. If you want to keep this here, there should at least be a word indicating that the config stuff is optional.
| # enabling networkx's config for nx-parallel | |
| # optional enabling networkx's config for nx-parallel |
There was a problem hiding this comment.
I don't think it is optional. We don't get any errors when we run with the default configs. But, we are also not running the algorithms on multiple cpu cores or threads because n_jobs=None by default. So, to actually run an algorithm as a parallel algorithm a user would have to set at least the n_jobs config to something non-None or an integer(excluding 0 and 1). I have updated the README.md accordingly.
There was a problem hiding this comment.
Wait -- the default value gives no parallel computation? That doesn't seem right to me. Shouldn't the default be to use all the cpus we can get?
There was a problem hiding this comment.
Wait -- the default value gives no parallel computation? That doesn't seem right to me.
I understand your concern. However, this behavior is consistent with joblib.parallel_config, where parallel computation isn’t enabled by default (i.e. n_jobs=None by default). Since modifying the defaults within joblib.parallel_config is outside my control(bcoz that would mean changing joblib's codebase), changing the default n_jobs in NetworkX’s configuration from None to -1 will introduce inconsistencies between the two configuration systems, and increase differences between the two config systems, leading to user confusion and potential pitfalls.
Shouldn't the default be to use all the cpus we can get?
That was indeed the previous default, but I've since updated the documentation to reflect the current behavior. Reverting this change would necessitate another extensive documentation update across the entire package, which might not be feasible before the GSoC deadline.
dschult
left a comment
There was a problem hiding this comment.
I approve of this PR.
I will make some suggested wording changes about when to use which config system once I figure out what they should be. But there is no point in delaying this PR for those changes.
Thanks very much @Schefflera-Arboricola for putting this together and guiding it through!!
This PR makes nx-parallel compatible with networkx's config system and documents how to configure nx-parallel using
joblib.parallel_config.In PR #68, I attempted to create a unified configuration system in
nx-parallelby wrapping thejoblib.Parallel()(inside nx-parallel algorithms) within awith joblib.parallel_config(configs)context, hereconfigsare extracted fromnx.config.backends.parallel. This approach made NetworkX’s config closely mirror joblib’s, giving the appearance of synchronization between the two systems. However, in the last meeting, Dan mentioned that this approach complicates things.In this PR, I tried to simplify things by clearly documenting both configuration methods for
nx-paralleland also added implementation to make nx-parallel compatible with NetworkX’s config. The_set_nx_configdecorator wrap the nx-parallel function call within ajoblib.parallel_configcontext only when NetworkX configs are enabled. If not, then configs set via joblib take precedence. (to make nx-parallel compatible withjoblib.parallel_configI just had to removen_jobsfrom the internaljoblib.Parallel)In the last meeting, Dan also questioned why we need to make nx-parallel compatible with networkx’s config and why can’t we simply use joblib to configure nx-parallel. And it is necessary to keep nx-parallel compatible with other networkx backends and backend functionalities. Like we would need this compatibility if we would want to change
backend_priorityand some backend configs in a context like this:refer
Config.mdfile in this PR for more.In the issue #76, I have clearly outlined the challenges in creating a unified configuration system in
nx-parallel.ref. PR #68 for more context
Thank you :)