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

Temp fix for relative import error #72

Closed
wants to merge 2 commits into from
Closed

Conversation

anirudh1666
Copy link
Collaborator

There is an import error in any of the demo notebooks, if you use barebones Jupyter Notebook (https://jupyter.org/.) Andy did not get this error when using PyCharm. We narrowed down the cause to the demos being unable to locate the kymata directory in the parent folder. Unfortunately, adding relative imports (ie, import kymata.x.y -> ..kymata.x.y) does not fix the issue.

This branch contains the hacky solution, which updates the PATH variable to point to the parent directory rather than the demos directory. This enables the script to find the required imports. We should look for a more user-friendly fix to this. One solution I found was: https://stackoverflow.com/questions/714063/importing-modules-from-parent-folder (solution #2.) This is quite tedious tho, so we can try and search for a better one.

@neukym neukym added ❓ discussion needed Extra discussion is needed before work can commence not-yet-ready-for-merging 🪲 bug Something isn't working labels Dec 2, 2023
@neukym
Copy link
Member

neukym commented Dec 21, 2023

from @anirudh1666: "Oh that is interesting. It looks like the path import error is a Windows issue. My mac doesn't have that error"

@neukym to look into before merging.

@neukym
Copy link
Member

neukym commented Dec 21, 2023

@anirudh1666 can you remove the label not-yet-ready-for-merge, add the label ready-to-merge and set me as the reviewer if you want me to review it pls?

@neukym
Copy link
Member

neukym commented Dec 22, 2023

@anirudh1666 - passing this to @caiw to review as he says he also has the same issue between OSX/windows.

Copy link
Member

@caiw caiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, @anirudh1666 is right this is the "correct" solution, but it looks like it might require that we finish making the toolbox into a proper package (aka #4).

In general I'm hesitant to put temp fixes in, but this one is not terrible (and it still for me on MacOS). Happy to either put this one in and add a note to #4 to undo it, or to hold off and focus on getting #4 done.

(Either way it would be good to get the other errors out of the notebook cell outputs)

@neukym
Copy link
Member

neukym commented Dec 22, 2023

Can I just confirm with you both that poetry doesn't sort this out automatically?

My understanding is that when we do "poetry install" it installs all the packages we need, plus an editable install of the project itself. i.e poetry is already implementing the pip/.egg/setup tools solution.

I say this because I remember @anirudh1666 saying he is not using poetry on Windows - so can you both use poetry and confirm relative imports for kymata still don't work?

@caiw
Copy link
Member

caiw commented Dec 23, 2023

Poetry does work with this for me, but I haven't actually encountered this exact error so I can't verify it's all that's required. I did do a relative editable install with poetry for making figures for the heeger paper and that worked fine.

@anirudh1666
Copy link
Collaborator Author

Hi All,

I checked this morning to see if the error still occurs with Poetry, and it still does. I agree with Cai that we can skip this merge, since once we package the project up, it will automatically be fixed. I'm going to be using mac as well now, so it shouldn't pose a problem for me.

So overall, I think we can focus on #4 and look at resolving this issue then.

@neukym
Copy link
Member

neukym commented Dec 25, 2023

Thanks for checking both. OK I will close this for now - we can always resurrect it if needed.

@neukym neukym closed this Dec 25, 2023
@caiw caiw mentioned this pull request Jan 12, 2024
@neukym neukym deleted the temp-import-fix branch February 19, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 bug Something isn't working ❓ discussion needed Extra discussion is needed before work can commence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants