schrodinger / pymol-open-source

Open-source foundation of the user-sponsored PyMOL molecular visualization system.
https://pymol.org/
Other
1.15k stars 275 forks source link

Use XDG directories instead of HOME to store plugins data and recent files cache #352

Closed jrom99 closed 4 months ago

pslacerda commented 6 months ago

I agree with your commit, however it must fallback to previous directory just in case anyone use still use it. But may be a good idea to do this in this on the 3.0 release not doing any any fallback.

JarrettSJohnson commented 5 months ago

Looks good. Will test later this week. @pslacerda @TstewDev , any further comments?

jrom99 commented 4 months ago

Any updates?

JarrettSJohnson commented 4 months ago

Sorry; other reviewers might've been busy. I'll just go ahead and merge. Thanks for the PR!

pslacerda commented 4 months ago

Hi, I'm also assuming this feature is alright, but want to test anyway.

I did a pip install -e . on my PyMOL directory and have it installed in my venv. However when I run pymol on the command line, it doesn't starts. The error I'm receiving is:

/pymol-open-source/venv/bin/python: can't open file '/pymol-open-source/venv/lib64/python3.12/site-packages/pymol/__init__.py': [Errno 2] No such file or directory
jrom99 commented 4 months ago

Hi, I'm also assuming this feature is alright, but want to test anyway.

I did a pip install -e . on my PyMOL directory and have it installed in my venv. However when I run pymol on the command line, it doesn't starts. The error I'm receiving is:

/pymol-open-source/venv/bin/python: can't open file '/pymol-open-source/venv/lib64/python3.12/site-packages/pymol/__init__.py': [Errno 2] No such file or directory

Did you specify a prefix (PREFIX-PATH=.) when installing? It seems that pymol was installed without it, and the path was truncated.

pslacerda commented 4 months ago

I just did the venv again an I got it working (mostly). Thank you @jrom99

pslacerda commented 4 months ago

Just tested and this PR doesn't work in the following case:

cmd.plugins.pref_set('a', 1)

It still save in ~/.pymolpluginsrc.py instead of the XDG directory.

pslacerda commented 4 months ago

Not sure if I'm right about this error, but I couldn't find any error on your code.

EDIT: I didn't see any mkdir in your code. It should fallback the old file but should also prefer the "XDG" file if none was found.

jrom99 commented 4 months ago

Not sure if I'm right about this error, but I couldn't find any error on your code.

EDIT: I didn't see any mkdir in your code. It should fallback the old file but should also prefer the "XDG" file if none was found.

That's the specified behavior, I use os.makedirs(data_dir, exist_ok=True) while getting the pymol plugin file. The default function to get db files also has a os.makedirs. I'll take a look to check if the plugins modules actually uses these functions to get this file, or if they assume its location.

jrom99 commented 4 months ago

Just tested and this PR doesn't work in the following case:

cmd.plugins.pref_set('a', 1)

It still save in ~/.pymolpluginsrc.py instead of the XDG directory.

Can you provide this file? I think the set_startup_path function may be to blame.

Edit: I'll try testing later this week since I have a lot of work to do using pymol and cannot break it in any way before then lol

pslacerda commented 4 months ago

Just pref_set without any pymolpluginsrc.py

pslacerda commented 4 months ago

@JarrettSJohnson, I suggest to revert this PR for a while until testings are done.

@jrom99 maybe to write unit tests for this feature is a good idea.

pslacerda commented 4 months ago

Edit: I'll try testing later this week since I have a lot of work to do using pymol and cannot break it in any way before then lol

@jrom99 you can use virtualenvs in order to not to break your current PyMOL installation. Is it useful?

pslacerda commented 4 months ago

ping @jrom99

jrom99 commented 4 months ago

Sorry for the late reply, I forgot lol. I'll test it today!

jrom99 commented 4 months ago

@JarrettSJohnson, I suggest to revert this PR for a while until testings are done.

@jrom99 maybe to write unit tests for this feature is a good idea.

I agree that the PR should be reverted for the time being. That said, I'm unable to replicate the bug, are you sure that ~/.pymolpluginsrc.py didn't exist before you ran the code?

However, I've found another bug: since XDG_DATA_DIR/pymol is created during initialization, if the folder is deleted, then pymol won't be able to write to these files (as it could before since ~ should always be available). I'm not sure if this is a valid use case to fix, since just recreating the file may hide this data loss.

image

image

pslacerda commented 4 months ago

Are you Brazillian also? Junior is a common surname here. And you use the prompt just like I used to, and a hidden virtualenv?!

in my opinion, if pymolpluginsrc.py doesn't exists, it should be created any time it is about to be written.

You're right, I tested the standard PyMOL, hadn't check-out your branch. Sorry, my bad...

jrom99 commented 4 months ago

Sim, também sou brasileiro (no meu caso Junior é nome mesmo lol).

Since I've deleted my fork, I'll recreate the edits and fix the mentioned bug (as well as improve handling if platformdirs is actually missing). Should I create a new pull request? I'm not sure if I can make one from here.

pslacerda commented 4 months ago

We're missing Windows directories structure like %APPDATA% or something. appdirs seems a good way to handle with cross platform directories. It's only about 600 lines of code. What do you think, @JarrettSJohnson?

Junior é seu primeiro nome?

jrom99 commented 4 months ago

We're missing Windows directories structure like %APPDATA% or something. appdirs seems a good way to handle with cross platform directories. It's only about 600 lines of code. What do you think, @ JarrettSJohnson?

I'm using (platformdirs)[https://pypi.org/project/platformdirs/] as a dependency since it's better maintained nowadays. That said, since pymol only has pyQT5 mentioned as a 3rd party dependency, I was not sure if I could/should just add one more.

I noticed that biopython, PIL and numpy are listed on the workflow build.yml, shouldn't they be added as install dependencies as well?

Junior é seu primeiro nome?

É sim!

pslacerda commented 4 months ago

Maybe Qt has a standard way to handle this... and has:

https://doc.qt.io/qt-6/qstandardpaths.html

Curioso primeiro nome =)

pslacerda commented 4 months ago

i don't know about others, but PIL is used for testing if generated images are equal to standard.

EDIT: please correct me if I'm wrong...

JarrettSJohnson commented 4 months ago

I would rather not add an additional dependency for something this small in scope. PIL and biopython are optional dependencies. We add them in build.yml for unit testing purposes, but they are not required to run minimum PyMOL, whereas PyQt is obviously needed.

jrom99 commented 4 months ago

Platformdirs is pretty established for cross-platform directories handling and it's really lightweight compared to other dependencies. I'm not sure what would be the downsides, since the installation script automatically deals with downloading it.

Is there a specific reason to make pymol have so few dependencies? I thought it was due to having to support older python versions, but the install script specifies python3.9, so this feels like unnecessarily reinventing the wheel.

pslacerda commented 4 months ago

QStandardPaths is the best option here.

jrom99 commented 4 months ago

I've updated it to use QStandardPaths, and from my testing it seems to be working as expected. I'm not sure if I can edit this pull request to use the new fork, though: https://github.com/schrodinger/pymol-open-source/compare/master...jrom99:pymol-open-source:master

Edit: I'm sure this is the wrong place to ask, but is it possible to enable fractional scaling or increase font size on pymol's interface? I've installed it on a hi-dpi device and while 1x is too small, 2x seems too big (for the seq_view, internal gui and the bottom command line). It is not listening to the "large text" accessibility option in Ubuntu.

pslacerda commented 4 months ago

writableLocation don't guarantee that the directory exists, are you creating it?

pslacerda commented 4 months ago

May you create tests? setTestModeEnabled can be used. I also rather not to call the variable xdg_data_dir because it isn't specific for XDG directories.

jrom99 commented 4 months ago

I've read how to create tests, but I'm not sure how to write them (I don't usually write tests for the apps I write) or where to place them (would tests/system/ be a good location?). The tests I did were just deleting files randomly while running the program. So I'm not sure I tested corner cases or something like that.

Some questions:

I also rather not to call the variable xdg_data_dir because it isn't specific for XDG directories.

I'm not sure about what to name them, since this is their explicit purpose on linux, do you have any suggestion?

pslacerda commented 4 months ago

I don't know very well, but you can create a file named e.g. api/preferences.py with a funcion test_set_pref(). Inside you use set_pref and check for the respective filesystem changes. You can install pytest on your virtualenv and run the tests with:

$ python -m pytest -s tests/api/preferences.py

You can create multiple functions starting with test_* to create multiple scenarios like ~/pymolpluginsrc.py exists or not.

You can always encapsulate your changes in a class (e.g. AppDataDirManager) and test this class isolated.

@JarrettSJohnson what do you think?

And I don't know how to test for the PyMOL initialization.

Now the questions:

the tests would need to be platform specific and I don't have a windows or apple machine.

You can test with QStandardPaths the contents of the file setting setTestModeEnabled(True) at the beginning of the test and setTestModeEnabled(False) at the end.

is it enough to set XDG_... variables from the python script itself? I'm not sure if this would be able to affect other instances of pymol.

I'd rather not set OS specific environment variables because the testing is done in other OSes also. Use QStandardPaths

I also rather not to call the variable xdg_data_dir because it isn't specific for XDG directories.

I'm not sure about what to name them, since this is their explicit purpose on linux, do you have any suggestion? It isn't specific to linux, your code will run on all OSes.

Maybel data_dir?


Here some examples of tests:

pslacerda commented 4 months ago

If you don't want to create tests, it's ok, I guess. I may just being annoying.

This part of PyMOL seems to receive only manual tests.

jrom99 commented 4 months ago

I'll try creating them!

pslacerda commented 4 months ago

Just a tip that may help, try to move the expanduser, directory creation, QStandardPaths.writableLocation, etc inside your function(s) (or class), so you can test almost everything just by handling the function. Then you can test more than just set_pref.

Testable code is better.