pymol-open-source icon indicating copy to clipboard operation
pymol-open-source copied to clipboard

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

Open jrom99 opened this issue 1 year ago • 1 comments

jrom99 avatar Mar 26 '24 19:03 jrom99

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.

pslacerda avatar Mar 27 '24 01:03 pslacerda

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

JarrettSJohnson avatar Apr 30 '24 20:04 JarrettSJohnson

Any updates?

jrom99 avatar May 20 '24 23:05 jrom99

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

JarrettSJohnson avatar May 21 '24 00:05 JarrettSJohnson

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

pslacerda avatar May 21 '24 22:05 pslacerda

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.

jrom99 avatar May 21 '24 22:05 jrom99

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

pslacerda avatar May 21 '24 22:05 pslacerda

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 avatar May 21 '24 23:05 pslacerda

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.

pslacerda avatar May 21 '24 23:05 pslacerda

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 avatar May 22 '24 00:05 jrom99

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

jrom99 avatar May 22 '24 00:05 jrom99

Just pref_set without any pymolpluginsrc.py

pslacerda avatar May 22 '24 08:05 pslacerda

@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 avatar May 23 '24 21:05 pslacerda

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 avatar May 23 '24 21:05 pslacerda

ping @jrom99

pslacerda avatar May 30 '24 22:05 pslacerda

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

jrom99 avatar May 31 '24 15:05 jrom99

@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

jrom99 avatar May 31 '24 16:05 jrom99

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...

pslacerda avatar May 31 '24 18:05 pslacerda

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.

jrom99 avatar May 31 '24 19:05 jrom99

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?

pslacerda avatar May 31 '24 19:05 pslacerda

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!

jrom99 avatar May 31 '24 19:05 jrom99

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

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

Curioso primeiro nome =)

pslacerda avatar May 31 '24 19:05 pslacerda

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...

pslacerda avatar May 31 '24 19:05 pslacerda

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.

JarrettSJohnson avatar May 31 '24 19:05 JarrettSJohnson

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.

jrom99 avatar May 31 '24 20:05 jrom99

QStandardPaths is the best option here.

pslacerda avatar May 31 '24 20:05 pslacerda

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.

jrom99 avatar May 31 '24 20:05 jrom99

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

pslacerda avatar May 31 '24 20:05 pslacerda

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.

pslacerda avatar May 31 '24 21:05 pslacerda

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:

  • the tests would need to be platform specific and I don't have a windows or apple machine.
  • 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 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?

jrom99 avatar May 31 '24 22:05 jrom99