Clarity on what Transport.normalize should do

[ryotatomioka]

Hi AiiDA team,

I am writing a new Transport plugin to allow Azure Blob Storage to be used, which would be useful when you want to run your calculations in the cloud.

However, I am not sure what Transport.normalize() is supposed to do and would appreciate if you could clarify. The reasons I am confused are:

1. The name normalize suggest that it should be idempotent.
2. However LocalTransport prepends curdir to the path which violates idempotence.
3. The baseclass documentation specifically mentions '...determine what the server is considering to be the "current folder"'.

So I am not sure how to proceed. It seems like Transport.getcwd() is a more obvious way to check the "current folder". Can we replace all such uses of Transport.normalize() with Transport.getcwd() and ensure that normalize() is idempotent? Otherwise, would it make sense to rename normalize() to something that doesn't imply idempotence?

[sphuber]

I didn't write the original Transport class (maybe @giovannipizzi did, so it would be good to have his feedback) but looking at it, I think the implementation of LocalTransport is indeed incorrect. I think the implementer simply forgot to check whether path is relative or not. If the path is relative (which the implementation seems to be assuming) it seems correct to prepend the curdir. However, when it is absolute, it should simply call realpath(path) and that's it. Maybe it should validate whether the resolved filepath actually exists.

The comment in the docstring of suggesting to use normalize to get the current folder seems indeed not ideal. When not specifying a path (so the default '.' gets used) it happens to give the correct answer, however, I agree with your statement that one should simply use the getcwd for this.

If @giovannipizzi can confirm this analysis, I am happy to submit a PR with a patch.

[sphuber]

As a further comment, I think that normalize was probably intended to provide the behavior of pathlib.Path.resolve. Its strict argument ties into my suggestion of validating the existence of the resolved path in my comment above. Probably by default it should not do the check and raise, but it would be a useful option to have.

[giovannipizzi]

Thanks @sphuber for the quick answer. Yes, the intended behaviour of normalize is the one of pathlib.Path.resolve.
I agree the docstring part on curdir is not very clear - we can remove it, even if I realise we might have taken it from paramiko :-D (https://docs.paramiko.org/en/stable/api/sftp.html#paramiko.sftp_client.SFTPClient.normalize)
One could also use the occasion to uniform the docstrings with those of the two core plugins for that method.

I would stress hoverer that the current method is idempotent and does the right thing, even if not obvious: os.path.join is clever, and if you join two paths and the second is absolute, only the second one is returned (as if you cd in each of the components, and not as if you just join the strings and then remove double slashes).

E.g.:

In [1]: import os

In [2]: os.path.join('/a/b', '/c/d')
Out[2]: '/c/d'

Maybe clarifying the docstring is enough, but maybe also adding a comment to the implementation of the local plugin can help reducing misunderstandings.

One important thing to note is that we also want to resolve symlinks, so

1. you need realpath, and
2. you need to do this not simply on the string, but actually ask the remote computer to normalize/resolve the path

[ryotatomioka]

Thank you @sphuber and @giovannipizzi for your quick replies!

Indeed I wasn't aware of the behavior of os.path.join().

But this raises another question about how we should generalize this method to "virtual" file systems, because I am not sure what the client wants to do with the returned value. The most canonical representation for blob storage is the full URL which looks like https://account_name.blob.core.windows.net/container_name/path/to/blob. I can return everything after the host name (i.e., /container_name/path/to/blob) or just /path/to/blob but it is sort of arbitrary.

Maybe could you clarify how this method is used by the client? Then I can decide what BlobTransport.normalize() should return.

[ltalirz]

Hi @ryotatomioka , a quick check of the aiida-core codebase shows that Transport.normalize is used exclusively inside the local.py (in copy) and ssh.py (in open and in isfile) transport plugins.

If you're implementing a new transport, it therefore seems safe to me to adapt it to your needs (others may comment if they disagree).

[giovannipizzi]

Indeed, AiiDA doesn't need it directly. I think the answer to your question should be the same as the one to: if do chdir, what do you represent as /? The objects in the container, or the list of all containers? I guess the choice is up to you but you then need to be consistent. On the other hand the transport (I think) assumes PosixPaths, so if you map them to URLs, be careful that there might not be a 1:1 correspondence in some edge cases (or e.g. you have to correctly urldecode strings etc.)

[ryotatomioka]

I have written below observations / recommendations based on my experience trying to write a new Transport class. I feel it might beyond the scope of this Q&A. Please feel free to suggest to move the discussion somewhere else if you find the discussion useful.

Assumption / Caveats

• I regard Transport an abstraction of remote data storage with the ability to execute a command remotely (in the context of the stored data)
• I have a rough understanding of how a Scheduler interacts with a Transport but I don't have a picture of how it is used in other places.
• My recommendation might be biased towards Blob storage (see below).

General observations / questions

• Transport seems to have too many public methods: This makes it hard to implement a new transport. In addition, some methods (e.g., normalize) seem to be only used by the transport itself.

• Too many implementation details are exposed publicly: Do we really need cd and mkdir? Do we need the concept of absolute path?

• Low-level file system operations like chmod, chown and mkdir are not supported by "virtual" file systems like blob storage. Full support for glob (*) also seems unnecessary. Maybe "list files by prefix" is good enough.

• Transport is stateful: In addition to the statefulness due to "current working directory", transport currently needs to be opened before it can be used. This is causing some inconsistency in the scheduler. I see some methods use with self.transport: ... but other methods uses the transport assuming that it is already open.

Recommendations

• Reduce the number of public methods: The minimum function a transport needs to perform are bulk upload from local to job-specific directory, download from a job-specific directory, copy between different job directories, list remote files (by prefix) and execution of a scheduler command in a job directory.

  • For example, put method may take localpath (file or directory), job_id (which job to put the files into), and remotepath (relative path from the root of the job-specific directory). I imagine put copies files recursively by default. Alternatively, job_id and remotepath can be combined into a single string like <job ID>/path/to/file but transport should be free to decide the actual directory structure (e.g., sharding).
  • Similarly exec_command_wait_bytes may take remotepath (e.g., <job ID>/path/to/dir) and command and it should virtually execute the command in the context of remotepath. For blob storage, I intend to copy the files to a local temporary directory and execute the command there.

• Hide implementation details: cd and mkdir can be avoided if we disallowed copying files one by one. I think we are already using a temporary folder on the local machine in prepare_for_submission. We can stage everything in a local directory and put everything recursively preserving directory structure (and permissions?). In addition, the absolute path of a file should be hidden from client because this is implementation dependent. I imagine that a virtual path of a file / directory on a computer will have the form <job ID>/path/to/file. Again how the data is stored (e.g., sharded etc) is an implementation detail that should be hidden.

• Remove low-level file system operations (cd, mkdir, chmod, chown, support for glob) as described above, these can be avoided (or hidden as implementation details) if we switch to bulk upload, download, and copy.

• If possible make transport stateless This will help clarify the contract between the client of Transport class. For example, the client will not need to worry if the transport is already opened or not.

[sphuber]

I think a discussion on the Transport abstraction and how we can improve it to allow more external sources of computing and storage to interface with AiiDA would be very useful. The current concept is indeed quite restrictive and for example the following assumption:

I regard Transport an abstraction of remote data storage with the ability to execute a command remotely (in the context of the stored data)

doesn't really apply. The Transport interface is currently assumed to not only allow commands to be executed remotely with respect to the data but also the compute platform, and it is actively used in that way by AiiDA's engine. For example, the transport is used to submit commands to the scheduler and execute bash commands on the remote computing resource. If I understand your use case correctly, it seems that here you are merely talking about data storage and the computation is being executed elsewhere. This might be a useful separation of concerns, but the current Transport was never designed in that way. So in order to support that, a redesign would most likely be useful if not necessary.

I think @ryotatomioka has given us a great start to discuss a use case that is clearly missing. I think setting up a meeting to discuss this in person would be more efficient than going back and forth in text here and I would be more than happy to join in.

[ltalirz]

Just as a follow-up on some specific suggestions:

• Transport.chmod: as far as I can see, this is currently used only once in the codebase, namely when transferring local codes. As pointed out before we are not aware of anyone who actually uses local codes, so I would not be against dropping this feature
• Transport.chown: as far as I can see, this is currently unused in the codebase. The function is also not tested

[ltalirz]

for example the following assumption doesn't really apply

I think you interpreted @ryotatomioka 's sentence a bit more narrowly than it was intended - "command execution in the context of the stored data" simply means running a command (could be submitting a script to a scheduler; copying files; ...) within the context of the data that has been copied and thus determines what these commands will do. I.e. our use case certainly involves both file transfer and job management.

However, AiiDA implicitly assumes that file system that is available to the compute resource is also mounted on the machine where the scheduler is running (for a HPC cluster typically the login node), and that executing Unix file system commands on the scheduler machine translate to the environment on the compute resource.
It is easy to imagine a case where the directory where the calculation runs is not mounted on the machine where the scheduler resides [1], and operations on those files need to go through a more restrictive API

[1] This will also be the case for aiida-fireworks, where the scheduler machine will typically be the machine where AiiDA is running (and submission happens through the local firecrest Python client). In this particular case, however, it seems like the firecrest API does expose many of the UNIX file system commands directly (e.g. chmod, chown ).

[sphuber]

I did understand that @ryotatomioka was describing a use case where the data stored would not necessarily be directly available on the file system where the eventual calculation will be executed. What I noted though is that this assumption is baked into AiiDA more or less; in the Transport interface itself, but especially in how it is used. So I think we actually agree on the situation here. What I was trying to say is that even if you were to successfully implement the Transport interface for the blob storage, it would still probably not be enough because AiiDA assumes that for a CalcJob you define one computational resource and assumes that input files are written using the transport to a place that will be automatically "found" from the directory on the compute resource where the job is run. That is why I was saying that it would be useful to go back and even think if the abstraction of the Transport (as it is currently understood by AiiDA) is still useful and not to convoluted and so doesn't allow a variety of use case (for example where storage and compute resource are not necessarily the same machine).

[ryotatomioka]

Thank you @sphuber @ltalirz @chrisjsewell for your replies.

Regarding your comment

assumes that input files are written using the transport to a place that will be automatically "found" from the directory on the compute resource where the job is run.

You are right to think that I am suggesting a separation of concerns. If we can make the transport actually only be responsible for managing the storage and making it available for the compute, we can make the scheduler responsible for making the data show up in the right place for the compute. Currently this is not possible because the transport attribute of the scheduler is not set when the scheduler is generating the submit script.

I don't know what the interface between the scheduler and the transport should look like. For the existing ssh-based transport, this is a no-op because it is already available.

For blob storage, I can imagine that the scheduler queries account name, container name, etc from the transport and then formats them in the right way.

[sphuber]

Transport.chmod: as far as I can see, this is currently used only once in the codebase, namely when transferring local codes. As pointed out before we are not aware of anyone who actually uses local codes, so I would not be against dropping this feature

That could even be replaced by simply requiring that the executable of a local Code is already executable. We could enforce this with a simple check when the Code is created.

[ryotatomioka]

I would like to mention that I have recently discovered fsspec package (documentation) which has implementations including sftp and Azure blob storage.

While I still believe going to a smaller implementation surface would improve maintainability, fsspec seems like a drop-in replacement for the transport implementations in AiiDA.

[sphuber]

Looks interesting indeed. But wanted to reiterate that, as it currently is defined, the Transport interface does not just file system operations, but is also used to execute arbitrary commands on a remote computer. Not sure if fsspec is also designed with this in mind.