-
Notifications
You must be signed in to change notification settings - Fork 1
Notebook for Chapter 11 (temporal graphML) #28
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
base: chap1
Are you sure you want to change the base?
Conversation
deusebio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @amarzullo24 ! This is indeed quite good! We need to resolve a few issues attached (especially linked to OpenTLP) and create a proper poetry environment before merging.
I'm happy to help to create the Poetry environment, after we consolidate and resolve the few major issues attached. In order to do this, it would also be super helpful if you could also export the full environemnt that you have on Google Colab, such that I can reproduce it using poetry
Chapter11/Temporal_GraphML.ipynb
Outdated
| "# Import necessary libraries and modules\n", | ||
| "import numpy as np\n", | ||
| "import torch\n", | ||
| "from TMF.TMF import TMF # Custom TMF implementation\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue I had a look at the repository. Unfortunately it does not include any dependency and/or constraints. Despite it provides a number of interesting implementations, it does not provide any stable environments and I'd rather just include the module (i.e. TMF.TMF) that we need given the impossibility of properly wiring this with poetry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why in the notebook I'm cloning the repo on the fly. Can I leave it as it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we clone the repo on the fly so that all the pipeline it's self contained.
Chapter11/Temporal_GraphML.ipynb
Outdated
| "source": [ | ||
| "# Unzip the sample graph\n", | ||
| "%cd /content/OpenTLP/Python\n", | ||
| "!cd data && unzip data.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo given we should probably copy and paste the relevant bits from the module, we can probably just curl the data.zip and unzip its content with pure python code, as much as it is done here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get, sorry! Are you asking to just do the unzip in pure python or there's something more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to use only Python to unzip
Chapter11/Temporal_GraphML.ipynb
Outdated
| "# install StellarGraph from this modified repo if running on Google Colab\n", | ||
| "import sys\n", | ||
| "if 'google.colab' in sys.modules:\n", | ||
| " !pip install git+https://github.com/VenkateshwaranB/stellargraph.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question Why are we not using the "official" stellar graph? is there any implementation in this fork that are relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The official StellarGraph repo has some problems with Colab (I think we can use the original one locally). Check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably because of the Python version, and that's why we had to pin the python version in other chapters to 3.8. I would be consistent and stick to Python 3.8 and the official stellargraph version here as well, unless we have strong reason (e.g. the other repo implementing some functionalities needed for temporal graph ML) to not do that
We can consider to bump the Python to 3.10 across and use a different version of stellargraph, but I would do it for all the chapters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Using StellarGraph from requirements
Chapter11/Temporal_GraphML.ipynb
Outdated
| " !pip install torch-sparse -f https://pytorch-geometric.com/whl/torch-{TORCH}+{CUDA}.html\n", | ||
| " !pip install torch-cluster -f https://pytorch-geometric.com/whl/torch-{TORCH}+{CUDA}.html\n", | ||
| " !pip install torch-spline-conv -f https://pytorch-geometric.com/whl/torch-{TORCH}+{CUDA}.html\n", | ||
| " !pip install torch-geometric" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo we should remove this and use proper poetry environments. I can take care of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is just for testing in Colab. I think the poetry environment should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
@amarzullo24 could you have a look at the comments? We still need to integrate this with poetry and enable testing in CI to merge this PR |
|
I can look more into Poetry integration, but I would need the export for the env, such that I don't have to figure out the versions of the packages myself, also with a trial-and-error. If you take care of making the notebook self-contained, by also curling external datasets, then we should be good |
Notebook for Chapter 11 (temporal graphML)
TODO: