Prepare release 0.2.1

[yakutovicha]

No description provided.

[ltalirz]

Thanks @yakutovicha

Would you mind including more explanation in the comment, including when/if the workaround can be removed?

Also, is there a way to test something in order to make sure this issue does not return?
So far the tests of the docker image are very minimal; testing a few python imports etc. might make sense as well?

[yakutovicha]

This PR is actually for the release, to merge the bugfix, and the new base image (again, with bug fixes).

The issue is discussed quilte in detail here: aiidateam/aiida-core#4339. I cannot know when it will be fixed by pip guys, honestly.

Also, is there a way to test something in order to make sure this issue does not return?

The issue popped up in the aiida-core image, when trying to import pymatgen, which requires ruamel.yaml and is installed as the dependency of aiida-core. I can add a test there when bumping the base aiida-prerequisites container.

[ltalirz]

This PR is actually for the release, to merge the bugfix, and the new base image (again, with bug fixes).

Right. I'm not quite sure what you are saying with this - do you mean I should therefore not review the changes?
If you feel that the code has already been reviewed, then it would be useful to comment on what you would like me to review here.

The issue is discussed quilte in detail here: aiidateam/aiida-core#4339. I cannot know when it will be fixed by pip guys, honestly.

Sure - this is a very long thread. What I am suggesting is that it would be useful to give someone who is looking through the code a quick way to decide whether the workaround is still needed.

I can add a test there when bumping the base aiida-prerequisites container.

Is it not possible to test it here?
E.g. to do some pip /conda install of rumel_yaml/... to check that things work as expected?

[yakutovicha]

Right. I'm not quite sure what you are saying with this - do you mean I should therefore not review the changes?

No, I don't mean this. I mean the following. When I work with a repository/package, I try to adhere to the gitflow principles. This implies that every new feature/bugfix/update will be introduced via PR from the feature branch to the develop branch. The release PRs are supposed to be done when developers agree on making a new release. And the release PRs go from a release branch (if there are changes to be made) to master, or, like in this particular case, from develop to master (because nothing additional needs to be done, when making a new release). When I assign collaborator for a review, I assume they know the code and they are qualified to answer two questions:

• Is it okay to do the release with the current state of the develop branch?
• Does the reviewer agree with the new version tag?

In a case reviewer disagrees with the first point, I close the PR and I add the missing changes. In case the reviewer agrees with both of them - they approve the PR and I go on. What I am strongly against of doing is introducing the changes in the release PR.

If you feel that the code has already been reviewed, then it would be useful to comment on what you would like me to review here

Following the gitflow strategy, I guess we are all trying to introduce the changes via PRs. Most of the time PRs are visible, already reviewed and contain (hopefully) clear title/description. One can check the PR status by clicking on the corresponding link accompanying the commit. It is hard for me to understand why even more additional information needs to be provided if we already spend time on making things easily available and clear.

Sure - this is a very long thread. What I am suggesting is that it would be useful to give someone who is looking through the code a quick way to decide whether the workaround is still needed.

We can close the PR and make a new PR to add those changes, or we can merge the PR and introduce those changes afterwards. Both are fine with me.

E.g. to do some pip /conda install of rumel_yaml/... to check that things work as expected?

I agree, we can add such a check in this repo. More specifically I would go for installing pymatgen and testing that it is importable. This is essentially the issue we have experienced.

[ltalirz]

What I am strongly against of doing is introducing the changes in the release PR.

Sure, I agree.

It is hard for me to understand why even more additional information needs to be provided if we already spend time on making things easily available and clear.

Well, just to explain it from the reviewer's point of view: I was not involved in the previous commits to develop and I get a review request for this PR. I therefore assumed I should look at the code and check it.

One can certainly also imagine taking a different approach here, which is what we tend to do in aiida-core, where one can be sure that all of the changes to develop have already been reviewed individually.
There, in a PR to master (which will come from a release branch), I can ignore all updates that already are in develop and limit myself to reviewing updates to the version number and the changelog, as in aiidateam/aiida-core#3934

I'm just saying that when I saw the request, I first thought that you had the first option in mind; then your comment made me think it was the second option.
In the second case, it's not really clear to me what I can contribute - I wasn't involved in the discussions of this bugfix and I couldn't say whether there are any pros or cons to releasing the current state of develop (if the commits were reviewed properly, then I guess it should be fine). From my personal point of view, in such a case you can just go ahead and don't need my approval to do so.

We can close the PR and make a new PR to add those changes, or we can merge the PR and introduce those changes afterwards. Both are fine with me.

Also both fine with me.

I agree, we can add such a check in this repo. More specifically I would go for installing pymatgen and testing that it is importable. This is essentially the issue we have experienced.

I think that would make sense - without an explicit test it is quite difficult for someone new looking at the code to understand what is being fixed by the workaround.

[greschd]

The issue is discussed quilte in detail here: aiidateam/aiida-core#4339. I cannot know when it will be fixed by pip guys, honestly.

Just to comment on this specific point: I would be really surprised if this was fixed by a change in pip, as from their perspective everything works as expected (the name canonicalization may not be too well-documented, but it is consistent with what PyPI is doing).
The issue here is that conda doesn't use the same canonicalization - so it's a "mixing conda and pip" kind of problem. The conda-forge channel has fixed this (just for this package) in conda-forge/ruamel_yaml-feedstock#68. Here we're using the default channel though, so I don't know if / when that will propagate there.