biocypher
Output modularisation #4
Replies: 0 comments 53 replies
-
|
I suggest to move to .drawio.svg files, stored on the biocypher repository, as soon as possible. For instance, https://github.com/biocypher/biocypher/blob/main/docs/write_mode.drawio.svg. We can create a subfolder for the diagrams in the docs folder. This will allow more seamless collaborative work on the diagrams, including making changes via PRs. @kpto FYI, this is where we now lead the current output architecture discussion. |
Beta Was this translation helpful? Give feedback.
-
|
@slobentanzer To view and edit a diagram stored in GitHub, the app draw.io needs to be installed in the organisation. I have requested an installation to biocypher, please check. Also I don't think a mega thread to discuss everything is manageable, this thread may serve as an index of other architectural discussions but a discussion for a specific part should has it's own thread created. Of course if you know a few tricks of GitHub that can address my concern, please share them with me :) |
Beta Was this translation helpful? Give feedback.
-
|
Please give more constructive feedback (what do you need, which permissions do I need to modify for you) |
Beta Was this translation helpful? Give feedback.
-
|
@slobentanzer You are the owner and only you can see what granular controls you have. If you ask me I can only guess permissions that allows me to create label and edit PR which is not constructive as well because I don't know how exactly are actions mapped to granular permissions in GitHub. I can do some research and then tell you though. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah that would be great. I'd prefer if Edwin and yourself were maintainers of these processes as well. I don't know, for instance, where the permissions need to be updated (Discussions are associated to a repository, so is it a repo-level setting or organisation-level); or, which exact roles/permissions would be best for a maintainer role. If you find out something, please let me know. :) |
Beta Was this translation helpful? Give feedback.
-
|
While I will still do some research, but regarding this matter, I don't think I should have any permission to do what I proposed. I don't think we should have permissions to edit any issues/PRs owned by others or otherwise the workflow cannot be enforced. The additional label should be created by you, the PR should be edited by @ecarrenolozano . Do you really mean you want us to be maintainers of this repo? |
Beta Was this translation helpful? Give feedback.
-
|
Thinking about roles is important no doubt, but you are conflating some things here. And yes, my medium-term goal is that you will be core contributors and maintainers/moderators to the extent that you can guide other contributors without my intervention. My points:
|
Beta Was this translation helpful? Give feedback.
-
|
In parallel to the work that @ryxx0811 has made until now, I propose this format to ground the requirements in such a way that the deliverables are a bit more specific. For future features this should be an input to essentially define what exactly should be developed. We can think in a way to complement the already made template (Issue: Add New Component). +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ NetworkX Output ComponentDescription: Add functionality to retrieve a NetworkX graph directly from BioCypher. Issues/Pull Requests related: #361, #358 Software Requirements Specification1. IntroductionPurpose: Implement functionality for users to create, edit, and save a NetworkX knowledge graph with the BioCypher framework. Support in-memory graph modifications (adding/removing nodes and edges) and enable persitency (disk or database). 2. Functional Requirements (FR)
3. Non-Functional Requirements (NFR)
4. External dependencies (ED)
5. Acceptance Criteria (AC)
Appendix. Modeling Diagrams
sequenceDiagram
participant Biocypher Core
participant Networkx KG
participant Pandas DF
participant Networkx Writer
participant DBMS
Biocypher Core ->> Networkx KG: add_nodes/add_edges
Networkx KG ->> Pandas DF: add_nodes/add_edges
Pandas DF ->> Pandas DF: add_table
Pandas DF ->> Networkx KG: dataframe[node]/dataframe[edge]
Networkx KG ->> Biocypher Core: dataframe[node]/dataframe[edge]
Biocypher Core ->> Networkx KG: get_KG
Networkx KG ->> Networkx KG: get_KG
Networkx KG ->> Biocypher Core: Knowledge Graph
Biocypher Core ->> DBMS: write_to_dbms(optional)
DBMS ->> DBMS: write
Biocypher Core ->> Networkx Writer: write_to_file(optional)
Networkx Writer ->> Networkx Writer: write
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks @ecarrenolozano. Fully agree that transparent definition of what to add / change is great. To clarify, you just pasted the sequence diagram that @ryxx0811 posted before, right? This does not reflect your points above. Should we draw one that does?
Can you clarify that point? The |
Beta Was this translation helpful? Give feedback.
-
|
I think one essential decision is the internal format we use for the KG. I think it would be useful to research what would be the most flexible and technically efficient internal representation of the graph that fulfils your above requirements. We have the
|
Beta Was this translation helpful? Give feedback.
-
Yes, this is just a copy paste to show the modeling diagrams (if they are added) in the format proposed. I need to update the reply with the updated diagrams.
Exactly, for adding/changing BioCypher features I could create a new template that involves the new format as a basis for proposing those changes. If you agree, I could do that.
I will look information about this. |
Beta Was this translation helpful? Give feedback.
-
|
@ecarrenolozano yes, agree to all points |
Beta Was this translation helpful? Give feedback.
-
|
I attach a diagram, as a complement for this discussion. |
Beta Was this translation helpful? Give feedback.
-
|
@ecarrenolozano this may not be self-explanatory. Can you add the conclusions from our discussion as well? |
Beta Was this translation helpful? Give feedback.
-
|
A bit late to comment but what is the internal representation for? Why isn't it just another output like others? An output is agnostic to destination but merely something complying to the interface, whether it in the end uses database or memory does not matter. You can have multiple outputs at the same time, a database connection output, a database import data output, a networkx output and a pandas output. Isn't it more configurable to users? |
Beta Was this translation helpful? Give feedback.
-
|
@slobentanzer It seems to me that you think a model, apart from data structure, must also defines relevant business logic. This is not true and indeed also a bad practice. Rather than this: class Entity:
self.propA = None
def businessA()
pass
def businessB()
pass
entity = Entity()
entity.businessA()Do this: from business import businessA, businessB
class Entity:
self.propA = None
entity = Entity()
processedEntity = businessA.process(entity)
processedEntity = businessB.proecss(processedEntity)Not only it makes your program more modularised by separating responsibilities, your logic is now testable, like this: entity = MockEntity()
processedEntity = businessA.process(entity)
processedEntity.assert_logic_is_correct()You may argue that I can still test the original design like this: entity = Entity()
entity.businessA()
assert(entity.someProp == truth)But if businessB requires an entity that is processed by businessA first, you can't unit test your methods because they must run together entity = Entity()
entity.businessA()
entity.businessB()
assert(entity.businessBRelatedProp == truth) // failed, but because of businessA or businessB?But with business logic externalised, you can have a mock entity which acts differently, like this class MockEntityJustForBusinessB:
self.quirkyPropForTestingBusinessB = None
processedEntity = businessB.process(entity)
processedEntity.assert_logic_is_correct() |
Beta Was this translation helpful? Give feedback.
-
|
@slobentanzer After I posted my comment above, I further researched a bit and it seems that your and my approaches are referred as def __post_init__(self):
"""
Check for reserved keywords.
"""
if ":TYPE" in self.properties.keys():
logger.debug(
"Keyword ':TYPE' is reserved for Neo4j. "
"Removing from properties.",
# "Renaming to 'type'."
)
# self.properties["type"] = self.properties[":TYPE"]
del self.properties[":TYPE"]
elif "id" in self.properties.keys():
logger.debug(
"Keyword 'id' is reserved for Neo4j. "
"Removing from properties.",
# "Renaming to 'type'."
)
# self.properties["type"] = self.properties[":TYPE"]
del self.properties["id"]
elif "_ID" in self.properties.keys():
logger.debug(
"Keyword '_ID' is reserved for Postgres. "
"Removing from properties.",
# "Renaming to 'type'."
)
# self.properties["type"] = self.properties[":TYPE"]
del self.properties["_ID"]Obviously if I don't use Postgres, the Postgres keywords check does not apply on me. If I follow your pattern to fix it, it would become elif self.dbms == "postgres" and "_ID" in self.properties.keys():
logger.debug(
"Keyword '_ID' is reserved for Postgres. "
"Removing from properties.",
# "Renaming to 'type'."
)
# self.properties["type"] = self.properties[":TYPE"]
del self.properties["_ID"]It gives a few issues:
But if the logic is externalised instead, I can do from biocypher import config
def generate_validator(config):
validators = []
if config["output"] == "protgres":
validators.append(ProtgresKeywordValidator())
if config["output"] == "neo4j":
validators.append(Neo4jKeywordValidator())
return CompositeValidator(validators)
validator = generate_validator(config)
validator.validate(nodes)Doing so I solved all issues above but obviously this kind of pattern is a stranger to you. I am actually also not sure the research packages I checked (pandas, scikit-learn, corneto) are a good reference. Most of research packages does not have a global state like BioCypher configuration. Can you think of any similar project that I can learn from? |
Beta Was this translation helpful? Give feedback.
-
|
@kpto this is fine, I am not attached to any patterns. I would be happy with that refactor. I think it is somewhat beside the point though: this is an internal process no user ever sees (and runs once, so we don't gain a lot by optimising this part, even if some conditional runs twice). We can refactor this as much as we want to make the code more maintainable and efficient. I said above:
I think this part of the argument is currently much more important than internal optimisations. I am happy to refactor the code to align with good practice, but the API discussion is somewhat independent of that. Please focus on the topic of the discussion and PR, which is the output modularisation and related changes in the user-facing API for now (modes such as |
Beta Was this translation helpful? Give feedback.
-
|
Re your comment further above: I am aware that dependency injection is a thing, and I am also aware that some of the codebase is not what software engineers would have written. I am happy to make changes to any of this, but we need to focus on the prioritised issues; we will not fix the entire codebase at once. I want to particularly avoid premature optimisation. Let's get back to discussing the modes and internal representation, as those are what matters for this discussion and PR. |
Beta Was this translation helpful? Give feedback.
-
|
Check also #81 and #76, I have been considering this for a while. No definitive answer yet. |
Beta Was this translation helpful? Give feedback.
-
|
Hi guys, Happy to interact again:
At this point, we can implement a suitable data structure for our needs such as the Adjacency Maps (using Python dictionaries, they are quite efficient). The problem could appear when we work with large graphs. In my little research, I noticed there are different libraries that build the knowledge graph using optimized routines in C/C++. Fortunately, all of the following list count with Python bindings to interact with those libraries. We can study the possibility of using one the following libraries for support all our graph construction: We need to define and do experiments in order to see what is the best approach. Using the data structure in native Python, or relying on external libraries that helps us with this task. What do you think? |
Beta Was this translation helpful? Give feedback.
-
|
@slobentanzer In your design how do I turn off the in-memory representation if I don't need it and I want to avoid high memory usage? |
Beta Was this translation helpful? Give feedback.
-
|
@kpto you would use one of the other modes; One of the first simple things we could do in terms of configuration is switch from the current way of definition to more explicit |
Beta Was this translation helpful? Give feedback.
-
|
@kpto it is also related to the API: if we had complete separation of only two modes, we could agree that we use |
Beta Was this translation helpful? Give feedback.
-
|
Here are some older considerations (from OmniPath) about using tuples and dictionaries: saezlab/pypath#205 (comment) A lot of the same reasoning went into the decision for using tuples as the stream into BioCypher from the adapters. Since this is the current solution, it is a reasonable baseline for the comparison we'll do. |
Beta Was this translation helpful? Give feedback.
-
|
I have created a new comment with my specs for this internal data structure (from a previous response): https://github.com/orgs/biocypher/discussions/377?sort=new#discussioncomment-11077185 |
Beta Was this translation helpful? Give feedback.
-
The internal data structure for representing the KGThe data structure should be the minimal feature-complete implementation of any knowledge graph we aim to model; the internal technical structure we use to represent nodes and edges. It should be agnostic to input and output formats (as far as feasible) and technically efficient, to allow a low memory footprint, fast IO, streaming, etc. We currently have two things that share this task in some regards, I think it would be cleaner to agree on one, but there are some complications.
How they relate: the tuples are more like a minimal "convention" that is not enforced in code apart from some error messages. They are technically very efficient, but less controlled. They are currently the input stream into BioCypher and are "before normalisation", i.e., they are "translated" by the _translate module into the actual KG components, based on the schema configuration. After translation, they are The current workflow is a tradeoff between simplicity of implementation (the tuples) and rigorous checks for alignment with the KG schema definition, ontologies, etc (the data classes). Particularly the existence of the _translate module, that takes care of aligning any input from any adapter with the ontologies used in the background, complicates the programming (but ideally makes the task of building a KG easier for the user). In perspective:
|
Beta Was this translation helpful? Give feedback.
-
If the From what I understand, the internal representation in the core layer will mostly be used for format conversion only such as A more practical approach would be that the data transmittance was done via simple interface. The use of simple object to transfer data from adapter to core to good I think, it's just that the tuple should be replaced by a more explicitly defined class. Even if a graph is needed in an adapter for whatever reason, in the end the graph should still be transmitted via the DTO.
It seems to me that you want to extend the scope of the KG schema from
For an advanced user who wants to customise the adapter:
I don't think we need meta programming when the above could be implemented without it unless you want performance optimisation which is too early in this stage, otherwise it just complicates the project. A side question, do you also plan to have one unified schema combining the BioCypher config and KG schema which ultimately defines how a KG is produced and BioCypher is merely a tool to manufacture it? |
Beta Was this translation helpful? Give feedback.
-
I had a similar idea once, but didn't have time to follow up. You would just create two classes for nodes and edges that have parameters < ID, label, properties > (and likewise for edges)? I don't understand what you suggest with the generic adapter; an adapter is just one modular part of a pipeline (which is the main unit of organisation in BioCypher). Since @ecarrenolozano asked a question that also confuses adapter and pipeline (https://github.com/orgs/biocypher/discussions/387#discussion-7400403), I think this needs to be better explained in the documentation. The individual adapters need to be agnostic of the downstream events. Their only purpose is to "know" the resource and harmonise the data stream to be conformal with what the core expects. The schema technically is not part of the adapter, although they need to be synchronised in order to yield a sensible KG (pipeline output). This synchronisation is what ultimately should be automatic (currently is manual). If you consider the graphical abstract on the home page of the docs, you can see this distinction (adapters, ontologies, and configuration are separate parts). Let's not get too deep into the metaprogramming discussion until we have clarified the basics. |
Beta Was this translation helpful? Give feedback.
-
Yes
An uber adapter that can read many formats and understands an unified KG schema so the mapping from source data (let's say columnar data but I want to be more generic here) to node/edge DTO is dynamically configured by the unified KG schema. It does everything you meantion:
But you propose an internal representation that could be used by the adapters which contradicts this request. |
Beta Was this translation helpful? Give feedback.
-
|
Re point 1: this is straightforward, but would have to be benchmarked in relation to the existing approach. Tuples are a very efficient data structure, and a class adds some significant overhead. We need to make sure we don't slow down the process even more, as for large DBs this already takes a while. Re point 2: this sounds interesting, and I'd be happy to discuss down the line. Seems like a subject that is some time in the future, anyways.
No, I do not propose that. I am saying that (in the simplest case) the data structure used by the adapters and the internal format could be the same thing, i.e., collections of tuples. This does not suggest entanglement. Deciding to store collections of tuples as the internal representation of the core does not imply that the output stream of the adapters needs to be in any way aligned, nor does it need to be bidirectional. We could have dedicated classes as an interface in the adapters and still represent the KG in the core as collections of tuples. Adapters: provide an output stream that is ingested by the core. Can be collections of tuples, or something else. Core: has an internal representation of a KG built from adapter output streams. Can be collections of tuples, or something else. It is always justified to start with the simplest implementation, and increase complexity only if warranted. Primitive types are simple and efficient in Python, and moving to more complex data structures should be thoroughly tested for need and justified by increased safety, user-friendliness, or other considerations. |
Beta Was this translation helpful? Give feedback.
-
|
See also this (empty) milestone from a while ago: https://github.com/biocypher/biocypher/milestone/4 |
Beta Was this translation helpful? Give feedback.
-
import networkx as nx
import time
from pympler import asizeof
from biocypher._logger import logger
import sys
import csvimport data from csv files#generate edges and nodes
with open('experiment/dataset_30_nodes_proteins.csv', mode='r') as file:
reader = csv.reader(file)
nodes = list(tuple(row) for row in reader)[1:]
with open('experiment/dataset_30_edges_interactions.csv', mode='r') as file:
reader = csv.reader(file)
edges = list(tuple(row) for row in reader)[1:]print(len(nodes))
print(len(edges))
print(nodes)
print(edges)translate nodes and edges#translate to BiocypherNode and BiocypherEdge
import ast
from biocypher._create import BioCypherNode,BioCypherEdge
tnodes = [
BioCypherNode(node_id=node[0],
node_label=node[1],
properties=ast.literal_eval(node[2]))
for node in nodes
]
tedges = [
BioCypherEdge(source_id=edge[1],
target_id=edge[2],
relationship_label=edge[3],
relationship_id=edge[0],
properties=ast.literal_eval(edge[4]))
for edge in edges
]
print(len(tnodes))
print(len(tedges))
print(tnodes)
print(tedges)Dictionary as KG (Adjaceny map)#using dictionary as internal representation
class BiocypherKG:
def __init__(self):
self._KG={}
def add_nodes(self,nodes):
for node in nodes:
node_id=node.get_id()
if node_id not in self._KG:
self._KG[node_id] = {}
self._KG[node_id]['edges'] = {}
self._KG[node_id]['attributes'] = \
{'prefered_id': node.get_preferred_id(),
'node_label': node.get_label(),
'properties': node.get_properties()}
def add_edges(self,edges):
for edge in edges:
source_id = edge.get_source_id()
target_id = edge.get_target_id()
if source_id in self._KG:
if target_id not in self._KG[source_id]['edges']:
self._KG[source_id]['edges'][target_id] = \
{'relationship_label': edge.get_label(),
'relationship_id':edge.get_id(),
'properties':edge.get_properties()}
else:
raise TypeError(f'Knowledge Graph has no {source_id} key. \
Call add_nodes() to add the {source_id} node and its properties.')
def get_KG(self):
return self._KG
def to_networkx(self):
G = nx.DiGraph()
for k1, v1 in self._KG.items():
for k2, v2 in v1.items():
if k2 == 'edges':
for k3, v3 in v2.items():
G.add_edge(k1, k3, **v3)
else:
G.add_node(k1, **v1['attributes'])
return Gbkd=BiocypherKG()
import timet_1=time.time()
bkd.add_nodes(tnodes)
bkd.add_edges(tedges)
bkd.to_networkx()
t_2=time.time()
KG=bkd.get_KG()
size=asizeof.asizeof(KG)
print(f'Time(dict):{(t_2-t_1)*1000} ms.')
print(f'Size of KG:{size/1024} kb.')Tuple(BiocypherNode, BiocypherEdge, BiocypherRelAsEdge) as KG#using collections of tuples(BiocypherNode, BiocypherEdge, BiocypherRelAsEdge) as internal representation
class BiocypherKG:
def __init__(self):
self.G=nx.DiGraph()
def add_nodes(self,nodes):
for node in nodes:
if node not in self.G:
self.G.add_node(
node.get_id(),
label=node.get_label(),
properties=node.get_properties()
)
def add_edges(self,edges):
for edge in edges:
if edge not in self.G:
self.G.add_edge(
edge.get_source_id(),
edge.get_target_id(),
relationship_label=edge.get_label(),
relationship_id=edge.get_id(),
properties=edge.get_properties()
)bkt=BiocypherKG()t_1=time.time()
bkt.add_nodes(tnodes)
bkt.add_edges(tedges)
t_2=time.time()
tsize=asizeof.asizeof(tnodes)+asizeof.asizeof(tedges)
size=asizeof.asizeof(nodes)+asizeof.asizeof(edges)
print(f'Time(collections oof tuples):{(t_2-t_1)*1000} ms.')
print(f'Size of KG (translated):{tsize/1024} kb.')
print(f'Size of KG :{size/1024} kb.')Results
@ecarrenolozano @slobentanzer The result of the experiment. One issue with using a dictionary as the internal representation is that the add_nodes function must be called whenever a edge with new nodes is added to the dictionary. This is because the dictionary uses node_id as the key in the adjacency map. |
Beta Was this translation helpful? Give feedback.
-
Output modularisation
Re #361 and discussions therein, we should clarify which architecture changes we need to make to streamline the output API to represent all BioCypher functions and yet have the simplest, most intuitive functionality for the user.
@kpto made the observation that isolation is needed (the core should not need to know about the output); @slobentanzer proposes to focus core representation on the basic tuple representation (three-element for nodes, five-element for edges), and to have independent output modules that can be chosen by some configuration / API. The exact nature of these configuration options and API choices are the subject of discussion here. For that, we work with diagrams for visualising the architecture.
Architecture Diagrams
We include and expand our catalog of UML diagrams. You can view the diagrams (just click below). However, if you want to contribute, join our Zulip Channel and request access to edit the files.
Beta Was this translation helpful? Give feedback.
All reactions