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

Improved error messages when a style is not found #1419

Merged
merged 8 commits into from May 30, 2019

Conversation

@akohlmey
Copy link
Member

commented Apr 8, 2019

Summary

The error messages of the type "unknown XXX style" seem to be confusing a number of users, when looking at how often this is asked about on the lammps-users mailing list. This PR tries to address this.
In a possible second step, the code composing the error message could be expanded to do approximate matches and suggest alternatives when when encountering a typo, but i am not sure, how much of a help that would be, and it would require some more effort and complication.
Here is an example output for a LAMMPS binary, where only the USER-OMP package is installed:

[akohlmey@crunch build]$ ./lmp
LAMMPS (29 Mar 2019)
  using 1 OpenMP thread(s) per MPI task
pair_style xxx
ERROR: Unrecognized pair style 'xxx' (src/force.cpp:261)
Last command: pair_style xxx
[akohlmey@crunch build]$ ./lmp 
LAMMPS (29 Mar 2019)
  using 1 OpenMP thread(s) per MPI task
pair_style lj/cut/gpu
ERROR: Unrecognized pair style 'lj/cut/gpu' is part of the GPU package which is not enabled in this LAMMPS binary. (src/force.cpp:261)
Last command: pair_style lj/cut/gpu
[akohlmey@crunch build]$ ./lmp 
LAMMPS (29 Mar 2019)
  using 1 OpenMP thread(s) per MPI task
pair_style coul/long/omp
ERROR: Unrecognized pair style 'coul/long/omp' is part of the USER-OMP package, but seems to be missing because of a dependency (src/force.cpp:261)
Last command: pair_style coul/long/omp
[akohlmey@crunch build]$ 

Related Issues

none so far

Author(s)

Axel Kohlmeyer (Temple U)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility

The error message now starts with "Unrecognized xxx style" instead of "Unknown xxx style", which seems more appropriate to me.

Implementation Notes

To achieve checking against which styles are available in non-installed packages, header files similar to the (installed) styles headers are created, but the contain all styles and only styles from packages. This is done by utilizing similar code constructs and implemented for both, conventional and CMake based, build schemes.

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

akohlmey added some commits Apr 8, 2019

@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

sorry, not seeing the value of this. If a user gets the error message
"unknown pair style xxx" there are only 2 possibilities. Either they
misspelled it, or they didn't compile with it. Either is simple to figure
out. The doc page tells what package it is in. Trying to auto-correct
what a user did or intuit what they wanted to do is also a sinkhole IMHO.

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

sorry, not seeing the value of this. If a user gets the error message
"unknown pair style xxx" there are only 2 possibilities. Either they
misspelled it, or they didn't compile with it. Either is simple to figure
out. The doc page tells what package it is in. Trying to auto-correct
what a user did or intuit what they wanted to do is also a sinkhole IMHO.

@sjplimp you are looking at this from the perspective of a developer and a person that is familiar with how to find documentation. most of the LAMMPS users do not fall into that category anymore.
in this case, i don't see why this particular change is a sinkhole, since it closely follows the steps of the style file generation, and thus the amount of work it causes in addition should be negligible.

please recall, that one of the most persistent kind of inquiries on lammps-users is that people are completely taken aback by the "unknown xxx style" error message and have no clue what to do next. please keep in mind, that many of them did not compile LAMMPS, or are using some automated, minimal effort scheme, so they are rarely even aware of the fact that there are "packages" in LAMMPS and that not all commands are always available. i attribute part of the confusion to the use of the word "unknown" which is not very indicative of what is going on. most of the times the style is "known", it is not just available. linking those unrecognized styles to their respective packages in the error message helps to separate the two cases, that you mention (is see that as a significant plus for an inexperienced "i am just a user" kind of person; but even have been stumped myself on occasion). and it can also tell about the even more confusing situation, that a package is installed, but the corresponding style is not available, because it is derived from a style in a package that is not installed. please note, that i am not try to auto-correct something here, but provide context to a rather vague error message. as a bonus, we can provide more specific answer when people report those errors on the mailing list.

i agree, that making suggestions for misspellings is probably not worth the effort and trying to correct them on the fly would be a very, very bad idea (anybody using auto-correct on the phone or word processing program can tell what embarrassing and meaningless changes can happen without noticing). but this change can help to reduce a significant amount of frustration and confusion, especially in new LAMMPS users.

i don't want to start a discussion about what has been or is or is going to be a sinkhole in LAMMPS. that is better done in person. but i strongly believe, that having more specific and descriptive error message, for some even with a reference where to look for more info, is as much an improvement to LAMMPS as are improvements to the algorithms and source code. and i believe i am not the only person that is unhappy with the many too generic error messages in LAMMPS, and the fact that many parts of the code are assuming, that a user only creates meaningful inputs or gets everything right the first time (even worse is on the programmer level, but that is another can of worms. and one too big to open now). i have some ideas how to improve the situation, but this is not the place to discuss them.

TL;DR summary:
you should not worry about this causing effort, because it is not causing effort to you and is as much automated and in need of maintenance as the system creating the style files. and you should not worry about wasting time, since it is my time that went into this. and i don't mind.

@Adrian-Diaz

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I think that suggesting the package is a nice formality since the act of recognition provides more integration with the whole code if it is a package mismanagement. Definitely shouldn't spend time trying to identify close misspellings however, that becomes a very cumbersome block of code with strings, since that's already the only possibility. Even if its not done for every message at this point it seems like a case of the more the merrier.

@athomps

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

  1. Making the error messages systematically more granular is a good thing.
  2. The code implementation is pretty complicated, but looks very solid, and is in the style of the system for creating style files.
  3. We should not try to detect close misspellings
@athomps

athomps approved these changes Apr 8, 2019

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

you should not worry about this causing effort, because it is not causing effort to you

This is not necessarily true. Every line of code has a cost of maintenance, and the burden will be passed on to other developers in the future (no developer works on the code forever).

@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

A few more thoughts ...

I'm trying to balance user needs with code volume and complexity. Basically I don't want
to add code for things that don't provide enough value to the user. I'm not
clear which side of the line this is on. The examples given in the PR header don't seem
to me to be of much value. The current code already does the first case. The other 2
for /gpu and /omp seem obvious. If a user doesn't know those files are in the GPU
and OMP packages, how are they going to run LAMMPS with those packages enabled
with their various options?

More generally if a user doesn't know how to find/use the command doc pages, I'm not really
seeing how they can write interesting input scripts. E.g. how to use a new pair
style w/out looking at the docs for the pair coeff args.
Ditto for using a fix or compute and their options. So telling
them to look at the command's doc page to find the package it's in doesn't seem onerous. If a new
user doesn't know how to do that yet, helping them learn how is a good thing.

So we could make a more wordy generic error message, e.g. unknown vs unrecogznized,
you either misspelled it of look on the command's doc page to fine the package, etc.

Or we could tell them what package the command is in. For all styles, for
both make and CMake. Is that what this code does? I'm not seeing how it
works for make, since the new packages() command in Make.sh looks similar
to style() which only searches installed files. Are you also somehow
searching thru all the un-installed files?
If that's what this code does (or where you're headed), I could see that being useful.

Re: non-satisfied dependency. I think that's a build problem, not a run-time problem.
Building with make never does that, correct? The dependencies get enforced each
time you include/exclude a package. Does CMake not do that? If so, I think that's
something that should be enforced by the CMake build, not by run-time error checking.

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

then you should (at least to a large part) welcome my change

It is more the attitude of "I'm going to do whatever I want because it doesn't affect you" that concerns me. Because it does affect me!

@@ -87,6 +87,7 @@ string(TOUPPER "${CMAKE_BUILD_TYPE}" BTYPE)
# this is fast, so check for it all the time
message(STATUS "Running check for auto-generated files from make-based build system")
file(GLOB SRC_AUTOGEN_FILES ${LAMMPS_SOURCE_DIR}/style_*.h)
file(GLOB SRC_AUTOGEN_FILES ${LAMMPS_SOURCE_DIR}/packages_*.h)

This comment has been minimized.

Copy link
@junghans

junghans Apr 8, 2019

Member

Isn't that overwriting the content of SRC_AUTOGEN_FILES from the line above?

This comment has been minimized.

Copy link
@akohlmey

akohlmey Apr 8, 2019

Author Member

yes it does. shouldn't be a big issue, but needs to be fixed. will sort it out ASAP.

This comment has been minimized.

Copy link
@akohlmey

akohlmey Apr 8, 2019

Author Member

done

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I agree that this error is too vague and comes up too often on the mail list. One time a fix in LAMMPS was moved from the main /src directory to a optional package, and one minute my input script ran fine, and another it crashed. That really confused me for a while, and a better error message could have helped. At the very least we should say to "check the spelling and look for missing optional packages".

akohlmey added some commits Apr 8, 2019

Merge branch 'master' into report-uninstalled-pkg-style
# Conflicts:
#	cmake/CMakeLists.txt
@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@akohlmey Unfortunately misreading someone’s words online isn’t hard to do when there is no tone of voice or non-verbal cues. To me, this:

“you should not worry about this causing effort, because it is not causing effort to you and is as much automated and in need of maintenance as the system creating the style files. and you should not worry about wasting time, since it is my time that went into this. and i don't mind.””

sort of sounds like this:

“I'm going to do whatever I want because it doesn't affect you”

Anyways, I just wanted to make sure that we are not adding complex code to core LAMMPS without considering that some other developer will have to maintain it in the future.

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@akohlmey Unfortunately misreading someone’s words online isn’t hard to do when there is no tone of voice or non-verbal cues. To me, this:

“you should not worry about this causing effort, because it is not causing effort to you and is as much automated and in need of maintenance as the system creating the style files. and you should not worry about wasting time, since it is my time that went into this. and i don't mind.””

sort of sounds like this:

“I'm going to do whatever I want because it doesn't affect you”

i was trying to point out, that i tried my best to keep the maintenance effort low, by following what is already done for the style_xxx.h files and trying to re-use as much code as possible. this part of LAMMPS has been extremely stable and with rather few changes over the years.

the changes to the force.cpp and modify.cpp and so on with the utility function, i consider a genuine improvement, so i would want to keep that, even if we cannot agree on storing and printing out the package name, if a style is missing in the executable because of a missing package or a missing package, the style depends on.

the rest is a one time effort of doing the implementation and testing it. it was more effort that i expected, but for a helpful (i think) improvement, i didn't mind that extra effort.

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

since this PR has had more concerns and questions than usual, i am not going to merge it until there are more approvals than we usually require. so please request changes to block the merge until your concerns are (mostly?) addressed or approve if you are (mostly?) OK with it.

@akohlmey

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

since there has been no response from @sjplimp on this, i take his vote as "abstain" and with the otherwise required approval from a second LAMMPS developer and no review with request for changes this is now going in.

@akohlmey akohlmey merged commit daac3f7 into lammps:master May 30, 2019

6 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details

@akohlmey akohlmey deleted the akohlmey:report-uninstalled-pkg-style branch May 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.