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

ORM: Fix typing of aiida.orm.nodes.data.code module #5830

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 8, 2022

Fixes #5819

@sphuber sphuber requested a review from chrisjsewell December 8, 2022 13:30
@sphuber sphuber force-pushed the fix/5819/orm-code-typing branch from b87d8d0 to 3889d66 Compare December 8, 2022 13:36
@chrisjsewell
Copy link
Member

Heya, so as I've mentioned before, the problem with using pathlib.PurePath here, is that it implies that one can also represent paths on Windows (aka DOS) systems, and I don't believe that is the case.

For starters, I don't think SshTransport is compatible with the remote computer being a Windows machine anyway.

Then for the code:

  1. Since aiida can only be installed on UNIX, this means that pathlib.PurePath will actually always be pathlib.PurePosixPath.
  2. Since the path is serialised to the storage as a string, even if the user would specifically add the executable path as PureWindowsPath, it would still end up being read as a unix path, e.g. PurePath(str(PureWindowsPath(r'C:\path\to\exec.exe'))).parent -> PurePosixPath('.') which is obviously wrong.
    • if you actually wantd to allow for windows paths, then you would need to specifically serialise the path somehow with specific information on the OS

So, basically, I think you should change all the PurePath uses to PurePosixPath

@sphuber sphuber force-pushed the fix/5819/orm-code-typing branch from 3889d66 to bf82003 Compare December 8, 2022 14:57
@sphuber
Copy link
Contributor Author

sphuber commented Dec 8, 2022

But what you are describing is a limitation of the Transport interface and the limitations other parts of AiiDA impose on the remote computer. But in principle the Code is abstract in that sense and should be able to support both no? It is possible that we implement support in the future for running on remote windows computers, and in that case the code classes should just work with PurePath. Is there any downside to keeping PurePath now?

The only thing maybe is if a user runs AiiDA on a windows machine (is this even possible?) uses pathlib.Path to construct the code instance and so (unwittingly) uses the wrong path type for a remote computer that is going to be POSIX-like. Still, since I don't think we support Windows, this shouldn't be an actual possibility.

@sphuber sphuber force-pushed the fix/5819/orm-code-typing branch from bf82003 to 44cb2ee Compare December 8, 2022 15:13
@chrisjsewell
Copy link
Member

The Code.executable_path is currently stored in the database as a string.
How do you know whether that string is intended to be posix or windows?
Currently it is essentially interpreted as the same as the filesystem that aiida is installed on, which is wrong.

@chrisjsewell
Copy link
Member

At present the problem is, by wrapping things in pathlib objects you have essentially made the Code less abstract than it previously was.

@sphuber
Copy link
Contributor Author

sphuber commented Dec 8, 2022

Currently it is essentially interpreted as the same as the filesystem that aiida is installed on, which is wrong.

Ah I see now, yeah you are right, that is a problem.

@sphuber sphuber force-pushed the fix/5819/orm-code-typing branch from 44cb2ee to 1b234a0 Compare December 8, 2022 16:33
The original type `PurePath` being returned by `filepath_executable` and
`get_execname` has been changed to `PurePosixPath` since currently the
rest of AiiDA's infrastructure requires/assumes that `Computer`s are
running POSIX operating systems.
@sphuber sphuber force-pushed the fix/5819/orm-code-typing branch from 1b234a0 to 158d331 Compare December 8, 2022 16:51
Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

cheers

@sphuber sphuber merged commit fdc676c into aiidateam:main Dec 9, 2022
@sphuber sphuber deleted the fix/5819/orm-code-typing branch December 9, 2022 09:47
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.

Fix type errors in orm.code modules
2 participants