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 XDG directories instead of HOME to store plugins data and recent files cache #352

Merged
merged 7 commits into from
May 21, 2024

Conversation

jrom99
Copy link
Contributor

@jrom99 jrom99 commented Mar 26, 2024

No description provided.

@pslacerda
Copy link
Contributor

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
Copy link
Member

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

@jrom99
Copy link
Contributor Author

jrom99 commented May 20, 2024

Any updates?

@JarrettSJohnson JarrettSJohnson merged commit b236007 into schrodinger:master May 21, 2024
3 checks passed
@JarrettSJohnson
Copy link
Member

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

@pslacerda
Copy link
Contributor

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
Copy link
Contributor Author

jrom99 commented May 21, 2024

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
Copy link
Contributor

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

@pslacerda
Copy link
Contributor

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
Copy link
Contributor

pslacerda commented May 21, 2024

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
Copy link
Contributor Author

jrom99 commented May 22, 2024

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
Copy link
Contributor Author

jrom99 commented May 22, 2024

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
Copy link
Contributor

pslacerda commented May 22, 2024

Just pref_set without any pymolpluginsrc.py

@pslacerda
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

ping @jrom99

@jrom99
Copy link
Contributor Author

jrom99 commented May 31, 2024

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

@jrom99
Copy link
Contributor Author

jrom99 commented May 31, 2024

@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

JarrettSJohnson added a commit that referenced this pull request May 31, 2024
@pslacerda
Copy link
Contributor

pslacerda commented May 31, 2024

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
Copy link
Contributor Author

jrom99 commented May 31, 2024

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
Copy link
Contributor

pslacerda commented May 31, 2024

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
Copy link
Contributor Author

jrom99 commented May 31, 2024

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
Copy link
Contributor

pslacerda commented May 31, 2024

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

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

Curioso primeiro nome =)

@pslacerda
Copy link
Contributor

pslacerda commented May 31, 2024

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
Copy link
Member

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
Copy link
Contributor Author

jrom99 commented May 31, 2024

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
Copy link
Contributor

QStandardPaths is the best option here.

@jrom99
Copy link
Contributor Author

jrom99 commented May 31, 2024

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: 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
Copy link
Contributor

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

@pslacerda
Copy link
Contributor

pslacerda commented May 31, 2024

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
Copy link
Contributor Author

jrom99 commented May 31, 2024

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?

@pslacerda
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

jrom99 commented Jun 1, 2024

I'll try creating them!

@pslacerda
Copy link
Contributor

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.

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 this pull request may close these issues.

3 participants