-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added support for the lexical query failure #565
Conversation
…tch GraphQL specs
|
||
update (config) { | ||
this.adapter = `${config.category}.${config.adapterName}` | ||
this.methodName = config.method | ||
|
||
this.message = `${this.message} (${this.adapter}.${this.methodName})` | ||
return this | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find it to be used anywhere so I've decided to remove the unused code. Please let me know if this code is needed and I will return it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be safer to leave it in, marked as deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the history of this part of code unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, I will do that (i.e. mark it as deprecated)
|
||
update (config) { | ||
this.adapter = `${config.category}.${config.adapterName}` | ||
this.methodName = config.method | ||
|
||
this.message = `${this.errorCode}: ${this.message} (${this.adapter}.${this.methodName})` | ||
return this | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same is above, I think it is not used any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above (leave in mark as deprecated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will mark it as deprecated.
const WarningCodes = { | ||
/* | ||
`hdwd` field of the response of the morphological adapter contains a number, not a string | ||
*/ | ||
NO_LEMMA_IN_HDWD: 'NO_LEMMA_IN_HDWD' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to have a centralized list of error codes. This way the client will know what kinds of errors to expect from the library (this file can be imported by the client app if necessary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but wonder about the best way to be consistent about how these are translated to text in the UI. Maybe we also need a locales file specific to errors and warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e.g. note that this particular warning is coupled with a retrieval of the message named MORPH_TRANSFORM_NO_LEMMA in the adapter code that uses this warning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question, we have two similiar classes now
AdapterError
and AdapterWarning
We use errorCode from the response for RemoteError
and errorCode for AdapterWarning
.
ErrorCodes for RemoteErrors you defined inside alpheios-messaging
ErrorCodes for Warnings you defined inside client-adapters
packages
So this way we need to know exactly the class source of the errorCode to be able to analyze it.
Because otherwises - it is only a String and not any short code.
May be it is better to think how to unify it the way you could retrieve all the data about the error from the error source? Similiar to Response errors from web server (404, 500 and so on)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having similar thoughts myself. I think we should somehow harmonize errors between different parts of the application, and think about localization of error messages. That could be a topic requiring some thought and discussion. It's probably better to create an issue for that. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth to be placed to a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
if (loading === false) { | ||
watchableQuery.stopPolling() | ||
} | ||
if (counter >= maxIterations) { | ||
watchableQuery.stopPolling() | ||
} | ||
counter++ | ||
dataCallback(data.word.homonyms, data.word.state, data.word.errors) | ||
dataCallback(data.word) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to pass the whole response the the calling function rather than parse it here: it might know better how to deal with the data returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
TUFTS_ERROR: 'TUFTS_ERROR', | ||
TREEBANK_ERROR: 'TREEBANK_ERROR', | ||
LEXICONS_ERROR: 'LEXICONS_ERROR', | ||
DISAMBIGUATION_ERROR: 'DISAMBIGUATION_ERROR', | ||
SHORT_DEFS_ERROR: 'SHORT_DEFS_ERROR' | ||
} | ||
export default WordQueryErrorCodes | ||
export default ErrorCodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this list of error codes can belong not to WordQuery only but can be shared across other queries. Hence the renaming.
CLIENT_ADAPTERS: 'ClientAdapters', | ||
DISAMBIGUATED_DATA_OBJECT: 'DisambiguatedDataObject' | ||
} | ||
export default ErrorOrigins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the modules where errors and warnings might be produced.
const ErrorSeverityTypes = { | ||
ERROR: 'Error', | ||
WARNING: 'Warning' | ||
} | ||
export default ErrorSeverityTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a way to distinguish between an error and a warning within the GraphQL error
object (as GraphQL specs do not support a warning
type).
} else if (clientAdaptersErrorOrWarning.constructor.name === 'AdapterWarning') { | ||
return new WordQueryWarning(...params) | ||
} else { | ||
throw new Error(`Unsupported error type: ${clientAdaptersErrorOrWarning.constructor.name}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need this for development to catch unsupported error objects (do not have any now). The exception will force us to fix the problem.
*/ | ||
constructor (message, code, warnData = {}) { | ||
super(message, code, warnData) | ||
this.extensions.severity = ErrorSeverityTypes.WARNING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as error, but with a different severity.
data: { | ||
homonyms: this.homonymGroup ? this.homonymGroup.homonyms.map(h => h.convertToJSONObject(true)) : [] | ||
}, | ||
extensions: { | ||
state: JSON.parse(JSON.stringify(this.state)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response format has been updated to match the latest GraphQL specs.
variables, | ||
source | ||
}) | ||
} catch (err) { | ||
Logger.getInstance().error('Observable word query error', err) | ||
} | ||
|
||
if (homonym) { | ||
if (wqData && wqData.homonym) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is something returned in the homonym
field, request succeeded fully or partially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good to me, I just had some small questions and change requests.
I'm also wondering if it's possible to add an integration test that tests this specific bug, i.e. creating a fixture in the alpheios-fixtures package, and testing that a query on 'caro' succeeds. I don't think we have integration tests for the new lexical query code yet, but having this sort of test for the inflection-tables has been enormously helpful (actually essential) to facilitating refactoring of that code. I think we need to start having similar tests here too. E.g. see the verified-issues.test.js in inflection-tables/tests/data
|
||
update (config) { | ||
this.adapter = `${config.category}.${config.adapterName}` | ||
this.methodName = config.method | ||
|
||
this.message = `${this.message} (${this.adapter}.${this.methodName})` | ||
return this | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be safer to leave it in, marked as deprecated.
|
||
update (config) { | ||
this.adapter = `${config.category}.${config.adapterName}` | ||
this.methodName = config.method | ||
|
||
this.message = `${this.errorCode}: ${this.message} (${this.adapter}.${this.methodName})` | ||
return this | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above (leave in mark as deprecated)
const WarningCodes = { | ||
/* | ||
`hdwd` field of the response of the morphological adapter contains a number, not a string | ||
*/ | ||
NO_LEMMA_IN_HDWD: 'NO_LEMMA_IN_HDWD' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but wonder about the best way to be consistent about how these are translated to text in the UI. Maybe we also need a locales file specific to errors and warnings?
const WarningCodes = { | ||
/* | ||
`hdwd` field of the response of the morphological adapter contains a number, not a string | ||
*/ | ||
NO_LEMMA_IN_HDWD: 'NO_LEMMA_IN_HDWD' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e.g. note that this particular warning is coupled with a retrieval of the message named MORPH_TRANSFORM_NO_LEMMA in the adapter code that uses this warning)
if (loading === false) { | ||
watchableQuery.stopPolling() | ||
} | ||
if (counter >= maxIterations) { | ||
watchableQuery.stopPolling() | ||
} | ||
counter++ | ||
dataCallback(data.word.homonyms, data.word.state, data.word.errors) | ||
dataCallback(data.word) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
@@ -14,34 +21,48 @@ export default class WordQueryError { | |||
* Creates an instance of WordQueryError. | |||
* | |||
* @param {string} message - An error message. | |||
* @param {import('./word-query-error-codes.js').WordQueryErrorCodes} code - An error code. | |||
* It is required to match Apollo GraphQL convention for errors. | |||
* @param {import('../../constants/error-codes.js').ErrorCodes} errCode - An error or a warning code. | |||
* @param {string[]} path - A path (a sequence of component names) where an error occurred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path is no longer a supported parameter here, right?
} | ||
this.message = message | ||
this.path = path | ||
this.path = ['word'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just noting that this is very different than the way the path was set before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to be more in line with the GraphQL specs. According to those, it should be a path within the GraphQL response to the elements that failed (see http://spec.graphql.org/draft/#example-90475 as an example). Since we do not have an exact element that failed (but may have that in the future) I decided to use the generic word
element. No looking at the example more closely I realize that it would be better to use homonyms
instead. I will do that change, if you don't mind.
Ideally we should identify the exact homonym and the lexeme that failed, but I'm not sure how easy will it be to do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do that change, if you don't mind.
ok thanks
@@ -181,14 +182,13 @@ class AlpheiosLexiconTransformer { | |||
// and not a String | |||
const lemmaText = elem.hdwd && elem.hdwd.$ ? `${elem.hdwd.$}` : '' | |||
if (!lemmaText) { | |||
this.adapter.addError(this.adapter.l10n.getMsg('MORPH_TRANSFORM_NO_LEMMA')) | |||
this.adapter.addWarning(WarningCodes.NO_LEMMA_IN_HDWD, this.adapter.l10n.getMsg('MORPH_TRANSFORM_NO_LEMMA')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also have to convert all of the other errors in this class to warnings rather than errors. (e.g. on lines 164 and 239)
@kirlat and @balmas, I have a question here about wordList workflow. For example (from embed-lib index.html), No, I am not sure, but I believe we didn't get any homonym and it is not saved to the wordlist now. |
The second case with Persian words. For now we lost this behaviour, I couldn't found my PR with this change ad couldn't even find the notice with such a text. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any code notes , but have some questions about the final result.
|
||
update (config) { | ||
this.adapter = `${config.category}.${config.adapterName}` | ||
this.methodName = config.method | ||
|
||
this.message = `${this.message} (${this.adapter}.${this.methodName})` | ||
return this | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember the history of this part of code unfortunately.
const WarningCodes = { | ||
/* | ||
`hdwd` field of the response of the morphological adapter contains a number, not a string | ||
*/ | ||
NO_LEMMA_IN_HDWD: 'NO_LEMMA_IN_HDWD' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question, we have two similiar classes now
AdapterError
and AdapterWarning
We use errorCode from the response for RemoteError
and errorCode for AdapterWarning
.
ErrorCodes for RemoteErrors you defined inside alpheios-messaging
ErrorCodes for Warnings you defined inside client-adapters
packages
So this way we need to know exactly the class source of the errorCode to be able to analyze it.
Because otherwises - it is only a String and not any short code.
May be it is better to think how to unify it the way you could retrieve all the data about the error from the error source? Similiar to Response errors from web server (404, 500 and so on)?
Thanks for catching this! I've missed those two cases during my testing. I will review them and see what's going on. I think we would need to create integration tests for them, if possible.
I don't remember removing it specifically. @irina060981, can you recall the number of your PR? I can try to track changes from it to see what happened with the code since then. |
…ecated, per the review suggestions
The upper progress bar is for the overall lexical query request. It disappears when the The lower progress bar is for short definitions. It disappears when either short definitions are retrieved or when we figure out that there are no short definitions for this word. @balmas, what would be the desired UI here? Both bars could make sense (especially if to change the wording a little bit), or we can show one or the other. We can, for example, use the upper bar for the lexeme retrieval and it can say something like "Lexemes are loading" and then the lower bar for short definitions ("Short definitions are loading"). First the upper bar will disappear and then the lower bar will follow suit, when short definitions' retrieval will be complete. This might give users a more detailed insight about what's going on behind the scenes. |
this is a good point. What should happen when you lookup a word without results is that the word should be added to the wordlist with itself as a lemma -- e.g. in these lines of the production lexical query, if we have no result form the morph adapter, we create a homonym with the form itself as the lemma: |
I would defer to @monzug on what should happen in the UI here. |
"lexical data is loading" message is generated while loading the morphology pop-up. when morphology pop-up is generated the message does not show up anymore. In the case of λεπτοψαμάθων it does briefly co-exist with the loading of the short definition progress bar in some cases (e.g. first look up of a Greek word) then it does quickly fade away. I am ok with it. |
|
I've examined the code carefully and it seems that the WordList controller does not listen to the In the LexicalQuery a Homonym is constructed even if the morph data request fails (at the point that Bridget referenced in the quote above), and then, a few lines below, this Homonym object used in the So it seems that in the current workflow, even if the morphological query failes, we still create a homonym from a target word nonethless nonetheless and, depending on the configuration, try to retrieve short definitions, full definitions, lemma translations, and word usage examples. Should we keep it this way? If so, for failed request the new workflow should create a Homonym, and pass it to the Lexical Query. Not sure how to be with short definitions in this case since those are retrieved behind the GraphQL facade. Should the GraphQL backend create a Homonym even if no morph data is available and try to retrieve short definitions for it? Analyzing the code, I've discovered that @balmas, @irina060981 please let me know what do you think about the desired workflow. |
Maybe we should change the text for the lower progress bar to |
Yes ! I think that's a very good idea. Thank you! |
Absolutely, YES
…On Thu, Dec 3, 2020 at 11:46 AM Bridget Almas ***@***.***> wrote:
"lexical data is loading" message is generated while loading the
morphology pop-up. when morphology pop-up is generated the message does not
show up anymore. In the case of λεπτοψαμάθων it does briefly co-exist with
the loading of the short definition progress bar in some cases (e.g. first
look up of a Greek word) then it does quickly fade away. I am ok with it.
Thanks @monzug <https://github.com/monzug> . So @kirlat
<https://github.com/kirlat> let's leave this for now.
Maybe we should change the text for the lower progress bar to Short
definitions are loading? It might be more understandable what this bar is
for this way. What do you think?
Yes ! I think that's a very good idea. Thank you!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#565 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJ32UOP6J4GZMODJFXY323LSS66HFANCNFSM4UK264MA>
.
|
Will update the text accordingly. |
Per discussion in Slack, the current production code is a bit of a hack, that allows us to continue to lookup a word in the additional resources (short definitions, full definitions, usage examples, etc) even if we fail to parse a lemma in the morphology service. I think we still want to do that, but I want to expand the data model so that we don't pretend the form is a "lemma" . This is described further in alpheios-project/documentation#33 Aside from making the code easier to understand and less error-prone, and supporting the additional use cases for the queries, our annotation workflows will require us to be explicit about what is being annotated, and we don't want to misrepresent an unparsed word form as a lemma as an annotation target. |
@balmas, @irina060981: do you think we can remove the |
I couldn't find the PR it was rather long ago.
We have a workflow for
I believe we could remove it. |
In our current workflow the |
As discussed with @balmas, I will merge the current PR with the current changes and two small additional fixes, and will implement other changes discussed as separate PR(s). This is to prevent discussion in the current PR from becoming too big. |
…rned from GraphQL queries
# Conflicts: # packages/client-adapters/dist/alpheios-client-adapters.js.map # packages/client-adapters/dist/alpheios-client-adapters.min.js # packages/client-adapters/dist/alpheios-client-adapters.node.js.map # packages/client-adapters/dist/alpheios-client-adapters.node.min.js # packages/components/dist/alpheios-components.js # packages/components/dist/alpheios-components.js.map # packages/components/dist/alpheios-components.min.js
This build fixes the #562 and adds some related improvements to the GraphQL API.
Here is the short summary of changes:
word
GraphQL query response has been updated according to the results of the discussion in morph adapter errors shouldn't fail the query if there are also valid results #562. If the query failed completely, then thehomonyms
field in the response will benull
and there will be errors in theerror
field. If the query failed only partially, then thehomonyms
field will contain homonyms data and theerrors
field will contain warnings.if (errCode ===A) {}
) and act accordingly. The good thing about error codes is that they can be transferred from object to object and even across the network.Please let me know what do you think.