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

Use relative (.xyz) and absolute (sssom.xyz) imports consistently #130

Open
matentzn opened this issue Aug 31, 2021 · 9 comments · Fixed by #142
Open

Use relative (.xyz) and absolute (sssom.xyz) imports consistently #130

matentzn opened this issue Aug 31, 2021 · 9 comments · Fixed by #142
Assignees

Comments

@matentzn
Copy link
Collaborator

See @cthoyt comment here.

@hrshdhgd hrshdhgd self-assigned this Sep 2, 2021
@hrshdhgd
Copy link
Contributor

hrshdhgd commented Sep 2, 2021

I've seen both used but sometimes using relative imports causes issues when a module is imported. Any other opinions?

@cthoyt
Copy link
Member

cthoyt commented Sep 12, 2021

I prefer relative imports because it reminds you never to run packaged code as a script, except through the python -m interface. There are no practical differences in what is possible, though I think that using absolute imports allows people to make nonsensical imports, like doing from sssom import ... in a very deeply nested module -> e.g., it allows you to not think about the code and this can get you into circular import trouble

cthoyt added a commit that referenced this issue Sep 12, 2021
1. Closes #130 by switching to all relative imports
2. Removes some dead code
3. Updates docstrings and type annotations
@cthoyt cthoyt mentioned this issue Sep 12, 2021
@matentzn
Copy link
Collaborator Author

I am happy either way, what you are saying makes sense, lets go with relative for now.. Thanks @cthoyt

cthoyt added a commit that referenced this issue Sep 13, 2021
* Code cleanup

1. Closes #130 by switching to all relative imports
2. Removes some dead code
3. Updates docstrings and type annotations

* More cleanup

1. Refactor the same validation code
2. Add more type annotations
3. standardize order of meta, curie map

* Update parsers.py

* Update text for output

* Update function calls and error types

* Update parsers.py

* Use meaningful variable names
@matentzn
Copy link
Collaborator Author

@joeflack4 had trouble getting sssom-py up and running for development.

Putting this decision up for another vote, here is the related SO thread:

https://stackoverflow.com/questions/4209641/absolute-vs-explicit-relative-import-of-python-module

@joeflack4 do you have any opinion?

@matentzn matentzn reopened this Feb 25, 2022
@joeflack4
Copy link
Collaborator

joeflack4 commented Feb 25, 2022

Thanks, @matentzn . That thread you linked is one of a great many on this topic. Like most of those threads, the consensus is to use absolute imports.

I've been having difficulty getting the relative imports to work with PyCharm, both in this project and other projects. As for this project, I'm using Python 3.9. I tried virtualenv, virtualenvwrapper, and tox. I've got my PyCharm interpreter set up correctly for each of these cases, and my debug config looks good to me (more information/screenshots in #209). I tried both with and without setting "sssom/` as "sources root" in PyCharm. So far I'm having no luck.


I've been working with Python for awhile and I wasn't sure if I agreed with @cthoyt . That doesn't seem to line up with my experience with Python. I've always used absolute imports when running my packages using python -m <package-name>. I was unaware if by design relative imports were supposed to be friendlier towards that? Not my experience.

I also just did an experiment locally, and as I expected, running a script directly which uses absolute imports did not work for me.

pkg
├── __init__.py
├── __main__.py
├── a.py
├── b.py
└── c.py
python3 pkg/c.py
Traceback (most recent call last):
  File "/Users/joeflack4/Desktop/pkg/c.py", line 1, in <module>
    from pkg.a import x
ModuleNotFoundError: No module named 'pkg'

However, running as a module this way works fine whether I'm using absolute or relative in this specific case:

pkg/__main__.py:

from .b import x
from pkg.a import z

print(x)
print(z)

I assigned both x and z to 1, so this is what I expected:

python3 -m pkg
1
1

In any case, as I mentioned to Nico and Harshad, I've tried relative imports a number of times, and while relative imports make a lot of sense to me in theory, and I would prefer to use them if I could, they don't end up working out for me in a lot of situations. In the past, I've gotten them to work in some situations, but then if I hand my code to someone else with a different machine / project setup, or if I try running my code a different way, they don't always work. Meanwhile, I've found absolute imports to be very reliable. I think I've gotten them to work just about always.

So, my recommendation is absolute imports (unless we can figure out some magic to make relative imports work reliably everywhere).

@cthoyt
Copy link
Member

cthoyt commented Mar 2, 2022

I don't have a lot of energy/motivation for education at the moment, but the fact that you ran into this issue demonstrates that if you use the non-src/ layout, it's possible to run code outside of a packaged context, which again I posit is bad because it enables the developer to forget about how others will use the code.

Best next step would be to adopt the standard src/ layout for this repo, which we've been talking about for a while.

@matentzn
Copy link
Collaborator Author

matentzn commented Mar 2, 2022

@joeflack4 are you positive you cannot fix the problem in your setup? We do not have the time atm to change the SSSOM layout, and I also do not want to hastily push absolute package imports through now without a proper investigation..

@joeflack4
Copy link
Collaborator

@matentzn As I mentioned (maybe in another thread), I'll just debug using absolute imports, and change them back to relative before I submit a PR.

@matentzn
Copy link
Collaborator Author

matentzn commented Mar 2, 2022

If it aint too much of a pain... :) You know best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants