-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fixing file construction #907
Conversation
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.
Let's merge this if there are no objections.
@@ -137,7 +137,7 @@ def locations(self): | |||
|
|||
def _setLFNnamePattern(self, lfn="", namePattern=""): | |||
|
|||
if self.defaultSE != "": | |||
if hasattr(self, 'defaultSE') and self.defaultSE != "": |
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.
Given that it's part of the schema why is this change necessary? wouldn't it be guaranteed to have this attribute?
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.
No, it's not there when you're reading in from an XML where the schema hasn't been written with the defaultSE
as the first attribute within the file. Also this could happen during object construction as the Schema is not an ordered dict and so the order in which the parameters arrive here is effectively random (we're just lucky that in 99% of cases it's sequential)
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.
Or even an XML where this parameter didn't exist as this will only be assigned to the object after the other XML attributes have been read in.
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 understand why the order is important as I was under the impression that as schema arguments these are all already present when __init__
is being called. Indeed that's why one could do something like:
def __init__(self, *kwargs):
self.defaultSE = kwargs.get('defaultSE')
Or indeed why an ordered dict for the schema is important as they are always retrieved via key rather than position.
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 object has a defined slot in the class as per Objects.py
.
The attribute is only correctly set via the class calling __init__
and initializing the attributes to the default value.
(I don't know if hasattr will pass when only the slot has been assigned in the metaclass)
However, when a new class is constructed via __new__
the class doesn't fully assign the attributes when constructing the object as they're not required.
Users are seeing:
ERROR XML from-file error for file:
Could not find attribute defaultSE in DiracFile(namePattern='', lfn='', localDir='None')
When attempting to load old jobs which don't have this attribute stored in the XML.
By checking for the attribute before calling it I think this should be enough to fix this bug and make old jobs accessible again.
Wrt the order of objects in the Schema I think the order that objects are constructed within the Objects.py
is such that the defaultSE
is assigned to the class first before the other attributes look for it, although I could be wrong.
Ideally things like this would go away if we made Schema objects handled via a defaultdict.
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.
hmmm this strikes me as a failing of the GangaObject
and ObjectMeta
classes as something like this is perfectly possible, see the simple example below.
class ObjectMeta(type):
def __new__(meta, name, bases, dct):
dct['__slots__'] = dct['schema'].keys()
return super(ObjectMeta, meta).__new__(meta, name, bases, dct)
class Object(object):
__metaclass__ = ObjectMeta
schema = {}
def __getattr__(self, attr):
if attr in self.__class__.schema:
return self.__class__.schema.get(attr)
return super(Object, self).__getattr__(attr)
class bob(Object):
schema = {'frank':'default'}
def __init__(self):
print self.frank
>>> b=bob()
default
Note: I realise that this will not properly work with multiple instances, it's a proof that these things can be pre-setup. My schema is just a dict
and so has no notion of instances whereas a more complicated Schema object could. Of course the proper way to do this is with properties which is what I did in the stuff that I was playing with
OK since this is a quick fix for a problem that is annoying for users otherwise I think it should be fine but it certainly highlights to me that I need to redouble my efforts to look at the schema.
You might like to think about reducing the conditional to the smaller but equivalent (I think) version below
- if hasattr(self, 'defaultSE') and self.defaultSE != "":
+ if getattr(self, 'defaultSE', '') != '':
@mesmith75 also I'd like to see this in 6.3.1 as it solves some problems users have with loading really old jobs. |
This should fix #903 whilst improving the exceptions thrown from the XML parser which until now are somewhat cryptic.