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

Standalone workspace #476

Merged
merged 14 commits into from
Aug 2, 2022
Merged

Conversation

riclarsson
Copy link
Contributor

@riclarsson riclarsson commented Jul 27, 2022

In short:

import pyarts
ws1 = pyarts.arts.Workspace()
ws2 = pyarts.arts.Workspace()

Now works as expected.

Features explained:
This makes each new instance of a Workspace independent of one-another. The global variables that get defined in workspace.cc are used only to initiate the state of a new Workspace instance. A new Workspace instance knows it is the original by storing a shared pointer to itself. All copies of a Workspace are shallow, and they will thus retain a copy of the shared pointer to their original instance.

Agenda instances have a shared pointer to the workspace that created them so that they may outlive the original Workspace. This shared pointer is used to ensure that you cannot copy an Agenda from one Workspace instance to another, or at least not execute any code properly if this was done.

In practice, we only have Workspace& and not std::shared_ptr<Workspace>& send in to methods. This means that Agenda instances created inside these methods actually get a non-owning shared pointer (std::shared_ptr(&workspace, [](Workspace*){})). A dud that doesn't delete. This means they can outlive the existence of the original Workspace. The solution seems to me to send in std::shared_ptr<Workspace>& to all methods. I will attempt this after I make the original draft pull request, but it is a massive overhaul to a lot of Arts files, and I am not sure how python will react.

@riclarsson
Copy link
Contributor Author

I didn't know about std::enable_shared_from_this when I started this. It would have been so so so much easier to do it that way. Anyways, what it does is that basically if you create a new instance of a Workspace using std::make_shared<Workspace>(), you can safely call its member function shared_ptr() to return a co-owning std::shared_ptr<Workspace>. If, however, you create a new Workspace instance using something like Workspace{}, a call to the shared_ptr() member function will throw an error.

What this means is that we want all instances of Workspace to have their memory owned by a shared pointer, because then all calls to member functions work. This creates only one remaining issue: OpenMP currently requires a copy of the workspace to not mess with the wrong variables. I've created a WorkspaceOmpGuard struct that deals with this for us from having a Workspace&, but I am not sure if this gives some sort of performance penalty. I don't think it should (and it should at the very least be less than the actual copy of Workspace we anyways perform before calling OpenMP). I think we can live with this weird guard, it basically changes code in parallel regions as

propmat_clearsky_agendaExecute(ws, ...                    // old style 
propmat_clearsky_agendaExecute(WorkspaceOmpGuard{ws}, ... // new style

where WorkspaceOmpGuard has a Workspace& operator and wraps a non-deleting shared pointer as owner.

Mainly fixing things related to `Workspace&` now having `shared_ptr()`,
so that `Agenda& (const)` can return a `Workspace& (const)` without
losing out on the properly co-owning pointer
@riclarsson riclarsson marked this pull request as ready for review August 1, 2022 08:07
@riclarsson riclarsson requested a review from olemke August 1, 2022 08:07
@olemke
Copy link
Member

olemke commented Aug 2, 2022

Fantastic! 👍

@olemke olemke merged commit 875dfea into atmtools:master Aug 2, 2022
@riclarsson riclarsson deleted the standalone_workspace branch August 2, 2022 09:11
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.

2 participants