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

Using aiida-fleur with develop versions of Fleur #128

Open
janssenhenning opened this issue Jun 22, 2021 · 8 comments
Open

Using aiida-fleur with develop versions of Fleur #128

janssenhenning opened this issue Jun 22, 2021 · 8 comments

Comments

@janssenhenning
Copy link
Contributor

This issue concerns both masci-tools and aiida-fleur, but I think aiida-fleur is the right place for this, since the issue is more apparent here

The file version of the current develop version is (and should be) higher than the latest available version in masci-tools. The reason for this is the easier distinction between invalid input files for the 5.1 release and files from the develop version, which just contain switches and inputs that are not in the release Schema. However, this requires some thought about how to use aiida-fleur with the develop version of fleur. Since the changes are very minor at the moment (only a couple optional switches that are new) We could enable this with two changes

First the FleurinpData, validates the XML files too early and does not make use of the fallback mechanism in the inpxml_parser. This is easy too fix and should be done.

The other place the user has to adjust is when modifying the fleurinpData. The first time you modify a develop version fleurinpData, you will need to set fleurInputVersion to 0.34

The bigger question is how this should be handled in general. At the moment this quick fix can be done and virtually everything still works. But when the schema has larger changes, there might be no way around forcing the user to insert the schema into their masci-tools installation. I'm strongly against adding the fleur schema to the masci-tools repo before it is fixed by a release, since this can only lead to incompatibilities and headaches down the road

@broeder-j
Copy link
Member

broeder-j commented Jun 22, 2021

The last release of masci-tools can only be compatible to the last release of fleur and maybe sometimes to develop.
In my view it would be fine to have the develop fleur schema always in masci-tools develop directly. (one has to think what happens on releases if we do this, or if that would bind masci-tools releases to fleur to avoid incompatibilities). So if you like to use the fleur develop branch with aiida-fleur you should also be on masci-tools develop.

I also understand that you are against this, but for small changes the schema it should not be a problem or?
One question would be: What has to be done on the masci-tools and fleur side to avoid these incompatibilities, i.e in terms of tests? If fleur would increase the version everytime for every schema change, i.e if they are bundled always on a branch and then merged into develop with a version increase, would that solve the problem? Then masci-tools will have support for schemas which are never in a fleur release, but that way it will also work with develop.

On the other side, these two fall backs you suggest for aiida-fleur I would do them anyway. And in the modification case we give a warning like 'I do not know this version yet, I try to fall back to latest version I know, which might lead to unexpected errors'.

@janssenhenning
Copy link
Contributor Author

You are probably right, that having the develop version of the schemas in masci-tools is not as bad as I made it out to be, but I have some doubts.

I don't think it's practical to restrict the masci-tools stable releases to the fleur releases, so that we never have develop schemas in stable masci-tools releases. There are just too many parts in masci-tools that have nothing to do with the fleur input/output that should not be influenced by this. This should be no problem if you always stay up to date with the newest aiida-fleur version since aiida-fleur can be more aligned with the fleur releases and raise the masci-tools requirement to make sure that the release version of the schema is in there. For older aiida-fleur versions this can of course not be guaranteed

Bundling the changes of the fleur schemas on fleur and raising the version everytime might help, but the problem I see is that changes to the schema can be quite far apart until they are not just adding one optional switch. So either you end up with having to raise the file version a lot of times to keep this consistent (which is an unnecessary maintenance burden on the fleur side I think) or you would need to hold up and bundle all these small changes to a bigger change, which could potentially delay/frustrate fleur developers who just want to add one small feature. I think for Max5 this was the right approach; we had a lot of changes shortly before the release and they were nicely bundled in a branch. But of course these changes were much more substantial

Also for people just working with masci-tools at the moment there is no immediately noticeable difference between a schema in a develop status and a fixed schema from a previous release. So if we decide to add the develop schemas,we definitely need a way of marking these clearly to be able to output warnings and so on.

But you are right for small changes this should be no problem. This can also be seen by the fact that at the moment the resetting of the fleurinputversion to 0.34 just solves all problems.

@janssenhenning
Copy link
Contributor Author

Okay I think I will start by putting in the two fallbacks I proposed, since Gregor wants to use the develop version of fleur with aiida-fleur 😄

@janssenhenning
Copy link
Contributor Author

@broeder-j After thinking about it for some more time I think we can solve this only in one place. The function load_inpxml on FleurinpData can check if it's own input version is the same as in the loaded schema dictionary (This is how this case is also detected in the parsers) and can ignore the validation errors in this case.

I would like to still output the validation errors as warnings. Do you happen to know if Data objects in aiida also have configured loggers?

@broeder-j
Copy link
Member

broeder-j commented Jun 25, 2021

Also I would like to run a nightly build, either on our CI, or on github against fleur develop, with the full workflow regression tests, because currently they are never run. Back then I ran them locally on my machine manually. Mocks of them are still to be enabled on the gitlab ci, with fake codes.
We could also test all input files of fleur regression tests, if we can create input for them. (ideal if the calculation through aiida gives the same result as the tests, but that I do not know howto do and is maybe overkill).

Also at some point we want to have calculation importers.

@janssenhenning: to the logging: not as the processes do, but i think if you would log something from a data object it would go to the daemon/aiida log, (or some other default aiida log file) before one could watch it live via verdi daemon logshow. Overall, I am not sure without checking and my knowledge is not up to date.

@janssenhenning
Copy link
Contributor Author

@broeder-j You are right there is a logger but it just relays the log messages to the notebook or the daemon depending, where it is run. It could be that after implementing it, there could be a lot of warnings thrown around. If there are too many warnings we can look into how to reduce the number of warnings

@janssenhenning
Copy link
Contributor Author

@broeder-j Probably outputting the complete validation errors is overkill, we can tell users to check the parser_info property since the validation errors will be in there. So a simple warning should be enough

@janssenhenning
Copy link
Contributor Author

@broeder-j Update to this: With the next masci-tools release (probably 0.7.0) it will be possible to add new Fleur schema files without changing any code and the parsers will run with warnings (with new click cli masci-tools fleur-schema add <path/to/schema file>). It is even possible if users configure it correctly to download the schema file from the iffgit instance with the command above.

I think with this we have both cases covered. People can use develop versions of the fleur code and aiida-fleur will parse the input/output files with warnings and as long as there are no larger changes everything will continue to work correctly (As can be seen in the regression tests on the fleur CI).

In the case of larger changes or the user wanting to use new features, which require new input, the schema can be added and the masci-tools out xml parser works under the assumption that nothing has changed dramatically.

And the last escalation is, where the out xml parser requires code changes. In this case it would require a new masci-tools release when the schema is fixed, which is reasonable is think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants