1511878618 commented 1 year ago

Reference Issues/PRs

No related issues/PRs

What does this implement/fix? Explain your changes

support for loading local pdb files by ProteinGraphDataset and InMemoryProteinGraphDataset.

What testing did you do to verify the changes in this PR?

python -m py.test tests/, a part result of testing is as below:

Pull Request Checklist

[x] Added a note about the modification or contribution to the ./CHANGELOG.md file (if applicable)
[x] Added appropriate unit test functions in the ./graphein/tests/* directories (if applicable)
[x] Modify documentation in the corresponding Jupyter Notebook under ./notebooks/ (if applicable)
[x] Ran python -m py.test tests/ and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., python -m py.test tests/protein/test_graphs.py)
[x] Checked for style issues by running black . and isort .

review-notebook-app[bot] commented 1 year ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

a-r-j commented 1 year ago

Thanks @1511878618 will give this a review.

@rg314 Paging for a consult on the failing docker build. It seems

RUN jupyter nbextension enable --py widgetsnbextension is throwing an error:

Jupyter command `jupyter-nbextension` not found.
The command '/bin/sh -c jupyter nbextension enable --py widgetsnbextension' returned a non-zero code: 1
Service 'graphein-cpu' failed to build : Build failed

Any ideas?

1511878618 commented 1 year ago

Ok, I comment the jupyter command since i could use ssh or vscode to connect the container. The reason of the bug, i guess, is that no commands in Dockerfile would install jupyter-nbextension. And in my view, it is not necessary quesstion since it is a support for extension of jupyter notebook while jupyter lab may be a better choice of it. I'll try to find which package for jupyter notebook and makes it works if we do use jupyter-nbextension.

------------------ 原始邮件 ------------------ 发件人: "a-r-j/graphein" @.>; 发送时间: 2022年9月14日(星期三) 晚上8:25 @.>; @.**@.>; 主题: Re: [a-r-j/graphein] Local dataset (PR #208)

Thanks @1511878618 will give this a review.

@rg314 Paging for a consult on the failing docker build. It seems

RUN jupyter nbextension enable --py widgetsnbextension is throwing an error: Jupyter command jupyter-nbextension not found. The command '/bin/sh -c jupyter nbextension enable --py widgetsnbextension' returned a non-zero code: 1 Service 'graphein-cpu' failed to build : Build failed
Any ideas?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

rg314 commented 1 year ago

jupyter command since i could use ssh or vscode to connect the container. The reason of the bug, i guess, is that no commands in Dockerfile would install jupyter-nbextension. And in my view, it is not necessary quesstion since it is a support for extension of jupyter notebook while jupyter lab

Hey @1511878618,

I agree that it's unnecessary however I'd kept it in so the example notebooks could be run in a containerized environment out of the box. The reason being it make it really easy for new users to quickly spin up a container and have the code running. As you say jupyter lab might be a better choice but then you'd need to test that the notebooks run in the containerized env. I would just fix the line rather than commenting it out.

I'll leave this up to @a-r-j

Let me know if you have any other questions.

1511878618 commented 1 year ago

Thanks @1511878618 will give this a review.

@rg314 Paging for a consult on the failing docker build. It seems

RUN jupyter nbextension enable --py widgetsnbextension is throwing an error:
Jupyter command `jupyter-nbextension` not found.
The command '/bin/sh -c jupyter nbextension enable --py widgetsnbextension' returned a non-zero code: 1
Service 'graphein-cpu' failed to build : Build failed
Any ideas?

I solve this like below:

And woooooops, i closed this comment. 🤣

I reopen it, and I commited the changes of Dockerfile, if you are interested it.

a-r-j commented 1 year ago

Thanks @rg314

I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).

I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

1511878618 commented 1 year ago

Thanks @rg314

I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).

I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows:

or just unfortunately, these package update and no longer need for nbextension 🤣
How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull), and so our environment is fixed,

a-r-j commented 1 year ago

How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull ), and so our environment is fixed

I'm not much of a docker user but this sounds smart. If you and @rg314 agree on this then I'm happy to do that

rg314 commented 1 year ago

Thanks @rg314 I'm inclined to leave it in (and it looks like @1511878618 has figured it out!). I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows:

or just unfortunately, these package update and no longer need for nbextension 🤣 How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull), and so our environment is fixed,

Off the top of my head I don't have a quick fix but I'm sure someone else has run into the same problem with the notebook. If you can fix by specifying the old version this would be good.

From which line are you suggesting to move to Docker hub? It maybe that in the future the version of pytorch needs to be bumped with means someone would need to manage that image on docker hub (not ideal).

If you can fix by setting the version that I can have a look in 3 hours.

1511878618 commented 1 year ago

Thanks @rg314 I'm inclined to leave it in (and it looks like @1511878618 has figured it out!). I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows:

or just unfortunately, these package update and no longer need for nbextension 🤣 How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull), and so our environment is fixed,

Off the top of my head I don't have a quick fix but I'm sure someone else has run into the same problem with the notebook. If you can fix by specifying the old version this would be good.

From which line are you suggesting to move to Docker hub? It maybe that in the future the version of pytorch needs to be bumped with means someone would need to manage that image on docker hub (not ideal).

If you can fix by setting the version that I can have a look in 3 hours.

Ok, i'm not sure whether i can find the specific version of each package. Hmmmm, i think the best choice in my mind is add pip install jupyter_contrib_nbextensions and maybe conda could do it, too.

Actually, i have no idea about how to manage the version of packages which are needed in our project and how to make sure it still works with latest version of them.

a-r-j commented 1 year ago

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

1511878618 commented 1 year ago

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

rg314 commented 1 year ago

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

Fixed in https://github.com/1511878618/graphein/pull/1 @1511878618. @a-r-j I also had to fix some bits with the SSL. I think there's something wrong with the SSL certs on regnetworkweb.org.

1511878618 commented 1 year ago

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

Fixed in 1511878618#1 @1511878618. @a-r-j I also had to fix some bits with the SSL. I think there's something wrong with the SSL certs on regnetworkweb.org.

Ok, Thanks a lot for this ! 👍 👍

codecov-commenter commented 1 year ago

Codecov Report

Merging #208 (4e3dd38) into master (8123f42) will increase coverage by 7.59%. The diff coverage is 51.22%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   40.27%   47.86%   +7.59%     
==========================================
  Files          48       92      +44     
  Lines        2811     5534    +2723     
==========================================
+ Hits         1132     2649    +1517     
- Misses       1679     2885    +1206

Impacted Files	Coverage Δ
graphein/grn/parse_trrust.py	`37.77% <ø> (ø)`
graphein/ml/diffusion.py	`0.00% <0.00%> (ø)`
graphein/ppi/edges.py	`100.00% <ø> (ø)`
graphein/ppi/graph_metadata.py	`0.00% <ø> (ø)`
graphein/ppi/graphs.py	`54.34% <ø> (ø)`
graphein/ppi/parse_biogrid.py	`75.00% <ø> (ø)`
graphein/ppi/visualisation.py	`0.00% <0.00%> (ø)`
graphein/protein/analysis.py	`0.00% <0.00%> (ø)`
graphein/protein/edges/intramolecular.py	`22.68% <0.00%> (ø)`
graphein/protein/features/sequence/utils.py	`28.00% <0.00%> (+3.00%)`	:arrow_up:
... and 80 more