Ticket #11460 (closed: fixed)
Update facilities schema for new JobManagerType attribute
Reported by: | Martyn Gigg | Owned by: | Federico M Pouzols |
---|---|---|---|
Priority: | blocker | Milestone: | Release 3.4 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Martyn Gigg |
Description
The system tests are failing validation of the Facilities.xml as the new JobManagerType attribute is unknown. We need to update the schema to understand the attribute
Change History
comment:1 follow-up: ↓ 5 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from new to inprogress
comment:2 Changed 6 years ago by Federico M Pouzols
Oops, I had totally missed this. It seems that the validation just passes or doesn't really happen on some platforms?
I noticed that the schema defines a few attributes that I'm pretty sure are never used in the code. Maybe we need a tiny design document/discussion about these parameters. I skipped that step as this was just adding one attribute, but in the future more attributes and/or children elements might be added (along the lines of the, currently unused/unknown configFileURL attribute).
comment:3 Changed 6 years ago by Federico Montesino Pouzols
add JobManagerType attribute of computeResource in facilities, re #11460
Changeset: 76e57fccdda5c048e1cfcea01667ff529fad891e
comment:4 Changed 6 years ago by Federico Montesino Pouzols
- Status changed from inprogress to verify
- Resolution set to fixed
This is being verified as pull request #501.
comment:5 in reply to: ↑ 1 Changed 6 years ago by Federico M Pouzols
Replying to Federico Montesino Pouzols:
add JobManagerType attribute of computeResource in facilities, re #11460
Changeset: ad3a81f7480a879e60f22618cff926dfce1cea2b
This commit had wiped-out the utf8 BOM that the file has at the beginning. Not sure what effects that could have, especially on win7. So I deleted it. Better to leave the file as it is. The following commit should be the good one.
comment:6 Changed 6 years ago by Martyn Gigg
- Status changed from verify to verifying
- Tester set to Martyn Gigg
comment:7 Changed 6 years ago by Martyn Gigg
The fix here isn't quite right. We've added an attribute not an element type so for this case the update should be a new <xs:attribute name="JobManagerType"/>.
Before we fix this though. I notice that there is already an optional type attribute that can be specified in the Facilities.xsd:31. Was this actually intended for this purpose? If so, then we should just starts using that instead?
comment:8 Changed 6 years ago by Federico Montesino Pouzols
My bad, thanks for spotting this. The attribute "type" is never used (as the elements configFileURL, mpirunExecutable, and pythonExecutable) and I had not seen it in the schema. I see two alternatives:
- Use "type"
- Replace "type" with "JobManagerType" or similar
I'd go for the second one, a more specific name. "type" could be confused with other parameters or variants of the schedulers.
I'm also wondering if we have any rules/conventions for capitalization, especially for the first letter, as I see different capitalizations for attributes in different places.
comment:9 Changed 6 years ago by Martyn Gigg
I agree that type is a bit generic so let's stick with something more specific here and replace type with jobmanagertype. We seem mostly consistent with XML attributes as all lowercase so let's keep that going.
comment:10 Changed 6 years ago by Federico Montesino Pouzols
fix schema, use jobmanagertype (lowercase) attr, re #11460
Changeset: 146e9e99a375df0aa80e582882008cc6f09dac56
comment:11 Changed 6 years ago by Federico Montesino Pouzols
use jobmanagertype attr name in parser and tests, re #11460
Changeset: f995b64b6b3819d443e7a24ffcd3093e40a1a936
comment:12 Changed 6 years ago by Federico Montesino Pouzols
put BOM back in, re #11460
Changeset: 947685613feb6841a52ec3d41619bde6af10ea28
comment:13 Changed 6 years ago by Federico Montesino Pouzols
Good, thanks for your comments. I think this is better now. I'm trying to make sure that everything is fine. But XMl validators apparently don't like our facilities schema (catalog complexType).
comment:14 Changed 6 years ago by Martyn Gigg
There is a ValidateFacilitiesFile system test that use some the genxmlif & minixsv python modules to do the validation
comment:15 Changed 6 years ago by Federico Montesino Pouzols
Aha, I had to install minixsv which was missing. I think several CI machines are also missing it, and the Validate... system tests are silently skipped.
comment:16 Changed 6 years ago by Martyn Gigg
I think you're right. It was installed on the rhel6 machines but not on rhel7
comment:17 Changed 6 years ago by Federico Montesino Pouzols
Yes, it seems so. Last night only rhel6 and win7 managed to catch this issue.
comment:18 Changed 6 years ago by Federico Montesino Pouzols
I'm now more confident that this is fine. All tests that I could run locally finished happily on win7 and debian, and I also did partial validations of the xml files with other tools.
We got an unrelated unit test failure on osx, a checkout failure on rhel7, and win7 is struggling to catch up with the pull_request build queue...
comment:19 Changed 6 years ago by Federico Montesino Pouzols
Jenkins, retest this please
comment:20 Changed 6 years ago by Martyn Gigg
Ok. I'll double check it locally and then merge it
comment:21 Changed 6 years ago by Martyn Gigg
The windows build failure was an unrelated doctest fail. I'm happy with this now.
comment:22 Changed 6 years ago by Martyn Gigg
- Status changed from verifying to closed
Merge pull request #501 from mantidproject/11460_update_facilities_schema_JobManagerType_attribute
Add JobManagerType attribute of computeResource in Facilities schema
Full changeset: 854fdd818ca36904e07bbfa5e65747ba1c6d7b4e
comment:23 Changed 5 years ago by Nick Draper
Somehow these slipped through without a resolution. Set to Fixed.
comment:24 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 12299
add JobManagerType attribute of computeResource in facilities, re #11460
Changeset: ad3a81f7480a879e60f22618cff926dfce1cea2b