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

Parsers: remove explicit check for the retrieved folder's existence #580

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Oct 9, 2020

Fixes #577

As of aiida-core==1.4.0, the engine itself will check for the presence
of the retrieved folder. If it is missing, which is really just a bug in
aiida-core the engine will terminate the process and not call the
parser. Anyway there would be no point since there is nothing to parse.
This means that now if the parser is called, it is guaranteed that the
retrieve folder exists and so we no longer have to explicitly check for
it.

Since this is only valid from aiida-core>=1.4 we update the dependency
requirement. We also require aiida-core>=1.4.2 because 1.4.0 and
1.4.1 contain a bug where the content of files in local_copy_list
are copied to the repository even though they shouldn't. For this plugin
this means that mostly the content of the pseudo potentials are
duplicated everytime in the repository, which will bloat the repository
unnecessarily.

@sphuber sphuber force-pushed the fix/577/remove-retrieved-check branch 2 times, most recently from dfbccf3 to 45edbf2 Compare October 9, 2020 19:40
@sphuber sphuber requested a review from greschd October 9, 2020 19:41
@greschd
Copy link
Member

greschd commented Oct 9, 2020

Hmm, I feel it's a bit early to remove 1.3 compatibility.. 1.4 is still relatively new. I may be biased though, since I haven't yet updated my production environment 😄

Anyway, maybe we could just add !=1.4.0,!=1.4.1 to the requirements now, and punt the rest of the changes for a bit? We can leave the PR as is, just put it on hold for a while.

@sphuber
Copy link
Contributor Author

sphuber commented Oct 9, 2020

Hmm, I feel it's a bit early to remove 1.3 compatibility.. 1.4 is still relatively new.

Fair enough, but this change requires 1.4. However, it is not pressing, so we can keep it open until we feel 1.4 is stable enough

@greschd
Copy link
Member

greschd commented Oct 9, 2020

Alright, thanks. Did I understand it right that the current code will work fine with 1.3 and 1.4, but at 1.4 what we're removing here is essentially dead code?

And just out of curiosity: Does aiida-core also use the 300 exit code for this, or something else?

@greschd greschd added the pr/on-hold PR should not be merged label Oct 9, 2020
@sphuber
Copy link
Contributor Author

sphuber commented Oct 9, 2020

Did I understand it right that the current code will work fine with 1.3 and 1.4, but at 1.4 what we're removing here is essentially dead code?

Yes

And just out of curiosity: Does aiida-core also use the 300 exit code for this, or something else?

It does define it, but it uses 100, as 1** is reserved for aiida-core internal exit codes:

https://github.com/aiidateam/aiida-core/blob/bd6903d88a4d88077150763574784a1bc375c644/aiida/engine/processes/calcjobs/calcjob.py#L197

I think it is fine, because the exit codes defined here in the plugin stem from the alpha days where I put them as a pre-caution.

@sphuber
Copy link
Contributor Author

sphuber commented Oct 13, 2020

@greschd although this change is not really pressing, the other PR #581 to add support for aiida-pseudo is pressing, for another project we are running. This also requires aiida-core~=1.4 for the support of extras on groups. I see your point that v1.4 is still relatively young, but this will just make develop require it. We still have aiida-quantumespresso==3.2 that is compatible with lower versions. So I would argue that it is fine to make develop require v1.4.

As of `aiida-core==1.4.0`, the engine itself will check for the presence
of the retrieved folder. If it is missing, which is really just a bug in
`aiida-core` the engine will terminate the process and not call the
parser. Anyway there would be no point since there is nothing to parse.
This means that now if the parser is called, it is guaranteed that the
retrieve folder exists and so we no longer have to explicitly check for
it.

Since this is only valid from `aiida-core>=1.4` we update the dependency
requirement. We also require `aiida-core>=1.4.2` because `1.4.0` and
`1.4.1` contain a bug where the content of files in `local_copy_list`
are copied to the repository even though they shouldn't. For this plugin
this means that mostly the content of the pseudo potentials are
duplicated everytime in the repository, which will bloat the repository
unnecessarily.
@sphuber sphuber force-pushed the fix/577/remove-retrieved-check branch from 45edbf2 to 0165ef4 Compare October 13, 2020 12:31
@greschd
Copy link
Member

greschd commented Oct 13, 2020

Right then, let's do this. I guess if I need to use develop in production I might as well update to 1.4.2.

@greschd greschd removed the pr/on-hold PR should not be merged label Oct 13, 2020
@sphuber sphuber merged commit 0db7978 into develop Oct 13, 2020
@sphuber sphuber deleted the fix/577/remove-retrieved-check branch October 13, 2020 12:41
sphuber added a commit that referenced this pull request Nov 17, 2020
…#580)

As of `aiida-core==1.4.0`, the engine itself will check for the presence
of the retrieved folder. If it is missing, which is really just a bug in
`aiida-core` the engine will terminate the process and not call the
parser. Anyway there would be no point since there is nothing to parse.
This means that now if the parser is called, it is guaranteed that the
retrieve folder exists and so we no longer have to explicitly check for
it.

Since this is only valid from `aiida-core>=1.4` we update the dependency
requirement. We also require `aiida-core>=1.4.2` because `1.4.0` and
`1.4.1` contain a bug where the content of files in `local_copy_list`
are copied to the repository even though they shouldn't. For this plugin
this means that mostly the content of the pseudo potentials are
duplicated every time in the repository, which will bloat the repository
unnecessarily.
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.

Remove explicit checking of existence of retrieved folders in the parsers
2 participants