Ticket #11460 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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

add JobManagerType attribute of computeResource in facilities, re #11460

Changeset: ad3a81f7480a879e60f22618cff926dfce1cea2b

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

Note: See TracTickets for help on using tickets.