[hg] galaxy 1557: Refactor internals of metadata.

classic Classic list List threaded Threaded
1 message Options
| Threaded
Open this post in threaded view
|

[hg] galaxy 1557: Refactor internals of metadata.

Nate Coraor (nate@bx.psu.edu)
details:   http://www.bx.psu.edu/hg/galaxy/rev/a5831435c654
changeset: 1557:a5831435c654
user:      Dan Blankenberg <[hidden email]>
date:      Thu Oct 16 14:12:37 2008 -0400
description:
Refactor internals of metadata.

Significantly reduce the number of objects created during interation (i.e. MetadataParameters are created only once per spec).

Some oddness remains until it can be determined to be necessary or not, i.e.:
iter( metadata_collection ) != iter( metadata_collection.items() )

10 file(s) affected in this change:

lib/galaxy/datatypes/data.py
lib/galaxy/datatypes/metadata.py
lib/galaxy/datatypes/sequence.py
lib/galaxy/datatypes/tabular.py
lib/galaxy/model/__init__.py
lib/galaxy/tools/__init__.py
lib/galaxy/tools/parameters/basic.py
lib/galaxy/tools/parameters/validation.py
lib/galaxy/web/controllers/root.py
templates/dataset/edit_attributes.mako

diffs (682 lines):

diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/datatypes/data.py
--- a/lib/galaxy/datatypes/data.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/datatypes/data.py Thu Oct 16 14:12:37 2008 -0400
@@ -1,9 +1,8 @@
 import logging, os, sys, time, sets, tempfile
 from galaxy import util
 from cgi import escape
-from galaxy.datatypes.metadata import *
-from galaxy.datatypes.metadata import MetadataElement
-from galaxy.datatypes import metadata
+import metadata
+from metadata import MetadataElement #import directly to maintain ease of use in Datatype class definitions
 
 log = logging.getLogger(__name__)
 
@@ -17,11 +16,11 @@
     Metaclass for Data class.  Sets up metadata spec.
     """
     def __init__( cls, name, bases, dict_ ):
-        cls.metadata_spec = MetadataSpecCollection()
-        for base in bases:
-            if hasattr(base, "metadata_spec"):
-                cls.metadata_spec.update(base.metadata_spec)
-        Statement.process( cls )
+        cls.metadata_spec = metadata.MetadataSpecCollection()
+        for base in bases: #loop through bases (class/types) of cls
+            if hasattr( base, "metadata_spec" ): #base of class Data (object) has no metadata
+                cls.metadata_spec.update( base.metadata_spec ) #add contents of metadata spec of base class to cls
+        metadata.Statement.process( cls )
 
 class Data( object ):
     """
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/datatypes/metadata.py
--- a/lib/galaxy/datatypes/metadata.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/datatypes/metadata.py Thu Oct 16 14:12:37 2008 -0400
@@ -1,296 +1,299 @@
 import sys, logging
 
-from galaxy.util.bunch import Bunch
+from galaxy.util import string_as_bool
 from galaxy.util.odict import odict
 from galaxy.web import form_builder
-from types import *
 
 log = logging.getLogger( __name__ )
 
-# Taken in part from Elixir and how they do it: http://elixir.ematia.de
-
-STATEMENTS = "__galaxy_statements__"
+STATEMENTS = "__galaxy_statements__" #this is the name of the property in a Datatype class where new metadata spec element Statements are stored
 
 class Statement( object ):
-    '''
+    """
     This class inserts its target into a list in the surrounding
     class.  the data.Data class has a metaclass which executes these
     statements.  This is how we shove the metadata element spec into
     the class.
-    '''
+    """
     def __init__( self, target ):
         self.target = target
     def __call__( self, *args, **kwargs ):
-        class_locals = sys._getframe(1).f_locals
-        statements = class_locals.setdefault(STATEMENTS, [])
-        statements.append((self, args, kwargs))
+        class_locals = sys._getframe( 1 ).f_locals #get the locals dictionary of the frame object one down in the call stack (i.e. the Datatype class calling MetadataElement)
+        statements = class_locals.setdefault( STATEMENTS, [] ) #get and set '__galaxy_statments__' to an empty list if not in locals dict
+        statements.append( ( self, args, kwargs ) ) #add Statement containing info to populate a MetadataElementSpec
     @classmethod
     def process( cls, element ):
         for statement, args, kwargs in getattr( element, STATEMENTS, [] ):
-            statement.target( element, *args, **kwargs )
+            statement.target( element, *args, **kwargs ) #statement.target is MetadataElementSpec, element is a Datatype class
+
+
+class MetadataCollection:
+    """
+    MetadataCollection is not a collection at all, but rather a proxy
+    to the real metadata which is stored as a Dictionary. This class
+    handles processing the metadata elements when they are set and
+    retrieved, returning default values in cases when metadata is not set.
+    """
+    def __init__(self, parent ):
+        self.parent = parent
+        #initialize dict if needed
+        if self.parent._metadata is None:
+            self.parent._metadata = {}
+    @property
+    def spec( self ):
+        return self.parent.datatype.metadata_spec
+    def __iter__( self ):
+        return self.parent._metadata.__iter__()
+    def get( self, key, default=None ):
+        try:
+            return self.__getattr__( key ) or default
+        except:
+            return default
+    def items(self):
+        return iter( [ ( k, self.get( k ) ) for k in self.spec.iterkeys() ] )
+    def __str__(self):
+        return dict( self.items() ).__str__()
+    def __nonzero__( self ):
+        return bool( self.parent._metadata )
+    def __getattr__( self, name ):
+        if name in self.spec:
+            if name in self.parent._metadata:
+                return self.spec[name].wrap( self.parent._metadata[name] )
+            return self.spec[name].wrap( self.spec[name].default )
+        if name in self.parent._metadata:
+            return self.parent._metadata[name]
+    def __setattr__( self, name, value ):
+        if name == "parent":
+            self.__dict__[name] = value
+        else:
+            if name in self.spec:
+                self.parent._metadata[name] = self.spec[name].unwrap( value )
+            else:
+                self.parent._metadata[name] = value
+    def element_is_set( self, name ):
+        return bool( self.parent._metadata.get( name, False ) )
+    def get_html_by_name( self, name, **kwd ):
+        if name in self.spec:
+            return self.spec[name].param.get_html( value=getattr( self, name ), context=self, **kwd )
+
 
 class MetadataSpecCollection( odict ):
-    '''
+    """
     A simple extension of dict which allows cleaner access to items
     and allows the values to be iterated over directly as if it were a
     list.  append() is also implemented for simplicity and does not
     "append".
-    '''
-    def __init__(self, dict = None):
-        odict.__init__(self, dict = None)
+    """
+    def __init__( self, dict = None ):
+        odict.__init__( self, dict = None )
     def append( self, item ):
         self[item.name] = item
     def iter( self ):
         return self.itervalues()
     def __getattr__( self, name ):
-        return self.get(name)
+        return self.get( name )
 
 class MetadataParameter( object ):
-    def __init__( self, spec, value, context ):
-        '''
+    def __init__( self, spec ):
+        self.spec = spec
+        
+    def get_html_field( self, value=None, context={}, other_values={}, **kwd ):
+        return form_builder.TextField( self.spec.name, value=value )
+    
+    def get_html( self, value, context={}, other_values={}, **kwd ):
+        """
         The "context" is simply the metadata collection/bunch holding
         this piece of metadata. This is passed in to allow for
         metadata to validate against each other (note: this could turn
         into a huge, recursive mess if not done with care). For
         example, a column assignment should validate against the
         number of columns in the dataset.
-        '''
-        self.spec = spec
-        """Since strings are stored as unicode, we need to decode them for display"""
-        if isinstance( value, ListType ):
-            for i, elem in enumerate( value ):
-                if type ( elem ) == unicode:
-                    value[i] = elem.decode( 'ascii' )
-        elif isinstance ( value, basestring ):
-            if type( value ) == unicode:
-                value = value.decode( 'ascii' )
-        self.value = value
-        self.context = context
-        self.display = True
-
-    def __str__(self):
-        if self.value is None:
-            return str(self.spec.no_value)
-        return str(self.value)
+        """
+        if self.spec.get("readonly"):
+            return value
+        if self.spec.get("optional"):
+            checked = False
+            if value: checked = "true"
+            checkbox = form_builder.CheckboxField( "is_" + self.spec.name, checked=checked )
+            return checkbox.get_html() + self.get_html_field( value=value, context=context, other_values=other_values, **kwd ).get_html()
+        else:
+            return self.get_html_field( value=value, context=context, other_values=other_values, **kwd ).get_html()
+    
+    def to_string( self, value ):
+        return str( value )
 
     @classmethod
-    def marshal( cls, value ):
-        '''
+    def marshal ( cls, value ):
+        """
         This method should/can be overridden to convert the incoming
         value to whatever type it is supposed to be.
-        '''
+        """
         return value
 
-    @classmethod
-    def validate( cls, value ):
-        '''
+    def validate( self, value ):
+        """
         Throw an exception if the value is invalid.
-        '''
+        """
         pass
 
-    def get_html_field( self, value=None, other_values={} ):
-        return form_builder.TextField( self.spec.name, value=value or self.value )
 
-    def get_html( self ):
-        if self.spec.get("readonly"):
-            return self.value
-        if self.spec.get("optional"):
-            checked = False
-            if self.value: checked = "true"
-            checkbox = form_builder.CheckboxField( "is_" + self.spec.name, checked=checked )
-            return checkbox.get_html() + self.get_html_field().get_html()
-        else:
-            return self.get_html_field().get_html()
-
-    @classmethod
-    def unwrap( cls, form_value ):
-        value = cls.marshal(form_value)
-        cls.validate(value)
+    def unwrap( self, form_value ):
+        """
+        Turns a value into its storable form.
+        """
+        value = self.marshal( form_value )
+        self.validate( value )
         return value
     
+    def wrap( self, value ):
+        """
+        Turns a value into its usable form.
+        """
+        return value
+
+
 class MetadataElementSpec( object ):
-    '''
+    """
     Defines a metadata element and adds it to the metadata_spec (which
     is a MetadataSpecCollection) of datatype.
-    '''
+    """
 
     def __init__( self, datatype, name=None, desc=None, param=MetadataParameter, default=None, no_value = None, visible=True, **kwargs ):
         self.name = name
         self.desc = desc or name
-        self.param = param
         self.default = default
         self.no_value = no_value
         self.visible = visible
         # Catch-all, allows for extra attributes to be set
         self.__dict__.update(kwargs)
-        datatype.metadata_spec.append( self )
+        #set up param last, as it uses values set above
+        self.param = param( self )
+        datatype.metadata_spec.append( self ) #add spec element to the spec
     def get( self, name ):
         return self.__dict__.get(name, None)
-    def hasAttribute( self, attribute ):
-        return ((self.permission & attribute) == attribute)
-    def wrap( self, value, context ):
-        return self.param( self, value, context )
-    def unwrap( self, form_value, context ):
-        return self.param.unwrap( form_value )
-            
+    def wrap( self, value ):
+        """
+        Turns a stored value into its usable form.
+        """
+        return self.param.wrap( value )
+    def unwrap( self, value ):
+        """
+        Turns an incoming value into its storable form.
+        """
+        return self.param.unwrap( value )
 
-# Basic attributes for describing metadata elements
-MetadataAttributes = Bunch(
-    READONLY = 1,
-    OPTIONAL = 2
-    )
-    
-
-class MetadataCollection:
-    """
-    MetadataCollection is not a collection at all, but rather a proxy
-    to the real metadata which is stored as a Bunch.  This class
-    handles updating the reference to the Bunch when it is changed (so
-    that SQLAlchemy knows to update it) as well as returning default
-    values in cases when metadata is not set.
-    """
-    def __init__(self, parent, spec):
-        self.parent = parent
-        if spec is None: self.spec = MetadataSpecCollection()
-        else: self.spec = spec
-        
-        #set default metadata values
-        if self.parent._metadata is None:
-            self.parent._metadata = {}
-        
-    def __iter__(self):
-        return self.bunch.__iter__()
-    def get( self, key, default=None ):
-        try:
-            return self.bunch.get( key, default ) or self.spec[key].default
-        except:
-            return default
-    def items(self):
-        return iter( [(k, self.get(k)) for k in self.spec.iterkeys() ] )
-    def __str__(self):
-        return dict( self.items() ).__str__()
-    def __nonzero__(self):
-        return self.bunch.__nonzero__()
-    def __getattr__(self, name):
-        if name == "bunch":
-            return self.parent._metadata
-        if name in self.bunch:
-            return self.bunch[name]
-        if name in self.spec:
-            return self.spec[name].default
-    def __setattr__(self, name, value):
-        if name in ["parent","spec"]:
-            self.__dict__[name] = value
-        elif name == "bunch":
-            self.parent._metadata = value
-        else:
-            self.bunch[name] = value
-    def element_is_set( self, name ):
-        return bool( self.bunch.get( name, False ) )
-
-MetadataElement = Statement(MetadataElementSpec)
-
+MetadataElement = Statement( MetadataElementSpec )
 
 """
 MetadataParameter sub-classes.
 """
 
 class SelectParameter( MetadataParameter ):
-    def __init__( self, spec, value, context ):
-        MetadataParameter.__init__( self, spec, value, context )
-        self.values = spec.get("values")
+    def __init__( self, spec ):
+        MetadataParameter.__init__( self, spec )
+        self.values = self.spec.get( "values" )
+        self.multiple = string_as_bool( self.spec.get( "multiple" ) )
     
-    def __setattr__(self, name, value):
-        MetadataParameter.__setattr__(self, name, value)
-        if name in ['value']:
-            if value is None:
-                MetadataParameter.__setattr__(self, name, [])
-            elif not isinstance(value, list):
-                MetadataParameter.__setattr__(self, name, [value])
+    def to_string( self, value ):
+        if value in [ None, [] ]:
+            return str( self.spec.no_value )
+        if not isinstance( value, list ):
+            value = [value]
+        return ",".join( map( str, value ) )
     
-    def __iter__( self ):
-        return iter( self.value )
-    
-    def __str__(self):
-        if self.value in [None, []]:
-            return str(self.spec.no_value)
-        return ",".join(map(str,self.value))
-    
-    def get_html_field( self, value=None, other_values={} ):
-        field = form_builder.SelectField( self.spec.name, multiple=self.spec.get("multiple"), display=self.spec.get("display") )
-        for value, label in self.values or [(value, value) for value in self.value]:
+    def get_html_field( self, value=None, context={}, other_values={}, values=None, **kwd ):
+        field = form_builder.SelectField( self.spec.name, multiple=self.multiple, display=self.spec.get("display") )
+        for val, label in self.values or values or [(val2, val2) for val2 in value]:
             try:
-                if value in self.value:
-                    field.add_option( label, value, selected=True )
+                if ( self.multiple and val in value ) or ( not self.multiple and val == value ):
+                    field.add_option( label, val, selected=True )
                 else:
-                    field.add_option( label, value, selected=False )
+                    field.add_option( label, val, selected=False )
             except TypeError:
-                field.add_option( value, label, selected=False )
+                field.add_option( val, label, selected=False )
 
         return field
 
-    def get_html( self ):
+    def get_html( self, value, context={}, other_values={}, values=None, **kwd ):
         if self.spec.get("readonly"):
-            if self.value in [None, [] ]:
-                return str(self.spec.no_value)
-            return ", ".join(map(str,self.value))
-        return MetadataParameter.get_html(self)
+            if value in [ None, [] ]:
+                return str( self.spec.no_value )
+            return ", ".join( map( str, value ) )
+        return MetadataParameter.get_html( self, value, context=context, other_values=other_values, values=values, **kwd )
+
+    def wrap( self, value ):
+        value = self.marshal( value ) #do we really need this (wasteful)? - yes because we are not sure that all existing selects have been stored previously as lists. Also this will handle the case where defaults/no_values are specified and are single non-list values.
+        if self.multiple:
+            return value
+        elif value:
+            return value[0] #single select, only return the first value
+        return None
 
     @classmethod
     def marshal( cls, value ):
         # Store select as list, even if single item
         if value is None: return []
-        if not isinstance(value, list): return [value]
+        if not isinstance( value, list ): return [value]
         return value
     
 class RangeParameter( SelectParameter ):
-    def __init__( self, spec, value, context ):
-        SelectParameter.__init__( self, spec, value, context )
+    def __init__( self, spec ):
+        SelectParameter.__init__( self, spec )
         # The spec must be set with min and max values
-        _min = spec.get("min") or 1
-        _max = spec.get("max") or 1
-        step = self.spec.get("step") or 1
-        self.values = zip(range( _min, _max, step ), range( _min, _max, step ))
+        self.min = spec.get( "min" ) or 1
+        self.max = spec.get( "max" ) or 1
+        self.step = self.spec.get( "step" ) or 1
 
+    def get_html_field( self, value=None, context={}, other_values={}, values=None, **kwd ):
+        if values is None:
+            values = zip( range( self.min, self.max, self.step ), range( self.min, self.max, self.step ))
+        return SelectParameter.get_html_field( self, value=value, context=context, other_values=other_values, values=values, **kwd )
+    
+    def get_html( self, value, context={}, other_values={}, values=None, **kwd ):
+        if values is None:
+            values = zip( range( self.min, self.max, self.step ), range( self.min, self.max, self.step ))
+        return SelectParameter.get_html( self, value, context=context, other_values=other_values, values=values, **kwd )
+    
     @classmethod
     def marshal( cls, value ):
-        values = [int(x) for x in value]
+        value = SelectParameter.marshal( value )
+        values = [ int(x) for x in value ]
         return values
     
 class ColumnParameter( RangeParameter ):
-    def __init__( self, spec, value, context ):
-        RangeParameter.__init__( self, spec, value, context )
-        column_range = range( 1, context.metadata.columns+1, 1 )
-        self.values = zip( column_range, column_range )
-        
-    @classmethod
-    def marshal( cls, value ):
-        return int(value)
-
+    
+    def get_html_field( self, value=None, context={}, other_values={}, values=None, **kwd ):
+        if values is None and context:
+            column_range = range( 1, context.columns+1, 1 )
+            values = zip( column_range, column_range )
+        return RangeParameter.get_html_field( self, value=value, context=context, other_values=other_values, values=values, **kwd )
+    
+    def get_html( self, value, context={}, other_values={}, values=None, **kwd ):
+        if values is None and context:
+            column_range = range( 1, context.columns+1, 1 )
+            values = zip( column_range, column_range )
+        return RangeParameter.get_html( self, value, context=context, other_values=other_values, values=values, **kwd )
+    
 class ColumnTypesParameter( MetadataParameter ):
-    def __init__( self, spec, value, context ):
-        MetadataParameter.__init__( self, spec, value, context )
-    def __str__(self):
-        return ",".join( map( str, self.value ) )
+    
+    def to_string( self, value ):
+        return ",".join( map( str, value ) )
 
 class PythonObjectParameter( MetadataParameter ):
-    def __init__( self, spec, value, context ):
-        MetadataParameter.__init__( self, spec, value, context )
-        self.value = value
-        self.display = False
+    def __init__( self, spec ):
+        MetadataParameter.__init__( self, spec )
     
-    def __str__(self):
-        if not self.value:
+    def to_string( self, value ):
+        if not value:
             return self.spec.to_string( self.spec.no_value )
-        return self.spec.to_string( self.value )
+        return self.spec.to_string( value )
     
-    def get_html_field( self, value=None, other_values={} ):
-        return form_builder.TextField( self.spec.name, value=str( self ) )
+    def get_html_field( self, value=None, context={}, other_values={}, **kwd ):
+        return form_builder.TextField( self.spec.name, value=self.to_string( value ) )
 
-    def get_html( self ):
+    def get_html( self, value=None, context={}, other_values={}, **kwd ):
         return str( self )
 
     @classmethod
     def marshal( cls, value ):
         return value
-
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/datatypes/sequence.py
--- a/lib/galaxy/datatypes/sequence.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/datatypes/sequence.py Thu Oct 16 14:12:37 2008 -0400
@@ -24,7 +24,7 @@
 
     """Add metadata elements"""
     MetadataElement( name="species", desc="Species", default=[], param=metadata.SelectParameter, multiple=True, readonly=True, no_value=None )
-    MetadataElement( name="species_chromosomes", desc="Species Chromosomes", value={}, param=metadata.PythonObjectParameter, readonly=True, no_value={}, to_string=str )
+    MetadataElement( name="species_chromosomes", desc="Species Chromosomes", value={}, param=metadata.PythonObjectParameter, readonly=True, no_value={}, to_string=str, visible=False )
 
 class Fasta( Sequence ):
     """Class representing a FASTA sequence"""
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/datatypes/tabular.py
--- a/lib/galaxy/datatypes/tabular.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/datatypes/tabular.py Thu Oct 16 14:12:37 2008 -0400
@@ -11,7 +11,6 @@
 from cgi import escape
 from galaxy.datatypes import metadata
 from galaxy.datatypes.metadata import MetadataElement
-from galaxy.datatypes.metadata import MetadataAttributes
 
 log = logging.getLogger(__name__)
 
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/model/__init__.py
--- a/lib/galaxy/model/__init__.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/model/__init__.py Thu Oct 16 14:12:37 2008 -0400
@@ -161,7 +161,7 @@
 
     def get_metadata( self ):
         if not hasattr( self, '_metadata_collection' ):
-            self._metadata_collection = MetadataCollection( self, self.datatype.metadata_spec )
+            self._metadata_collection = MetadataCollection( self )
         return self._metadata_collection
     def set_metadata( self, bunch ):
         # Needs to accept a MetadataCollection, a bunch, or a dict
@@ -192,8 +192,6 @@
 
     def change_datatype( self, new_ext ):
         self.clear_associated_files()
-        if hasattr( self, '_metadata_collection' ):
-            del self._metadata_collection
         datatypes_registry.change_datatype( self, new_ext )
     def get_size( self ):
         """Returns the size of the data on disk"""
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/tools/__init__.py
--- a/lib/galaxy/tools/__init__.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/tools/__init__.py Thu Oct 16 14:12:37 2008 -0400
@@ -1313,7 +1313,8 @@
             if name in self.metadata.spec:
                 if rval is None:
                     rval = self.metadata.spec[name].no_value
-                rval = self.metadata.spec[name].wrap( rval, self.metadata.parent )
+                rval = self.metadata.spec[name].param.to_string( rval )
+                setattr( self, name, rval ) #lets store this value, so we don't need to recalculate if needed again
             return rval
         def __nonzero__( self ):
             return self.metadata.__nonzero__()
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/tools/parameters/basic.py
--- a/lib/galaxy/tools/parameters/basic.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/tools/parameters/basic.py Thu Oct 16 14:12:37 2008 -0400
@@ -830,7 +830,7 @@
                 dataset = other_values[filter_key]
                 if dataset:
                     for meta_key, meta_dict in filter_value.iteritems():
-                        if ",".join( dataset.metadata.get( meta_key ) ) == meta_dict['value']:
+                        if dataset.metadata.spec[meta_key].param.to_string( dataset.metadata.get( meta_key ) ) == meta_dict['value']:
                             options.extend( meta_dict['options'] )
             return options
         return self.options
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/tools/parameters/validation.py
--- a/lib/galaxy/tools/parameters/validation.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/tools/parameters/validation.py Thu Oct 16 14:12:37 2008 -0400
@@ -251,7 +251,7 @@
     def validate( self, value, history = None ):
         if not value: return
         if hasattr( value, "metadata" ):
-            if str( getattr( value.datatype.metadata_spec, self.metadata_name ).wrap( value.metadata.get( self.metadata_name ), value ) ) in self.valid_values:
+            if value.metadata.spec[self.metadata_name].param.to_string( value.metadata.get( self.metadata_name ) ) in self.valid_values:
                 return
         raise ValueError( self.message )
 
diff -r 97a8f6f5ba00 -r a5831435c654 lib/galaxy/web/controllers/root.py
--- a/lib/galaxy/web/controllers/root.py Wed Oct 15 15:51:13 2008 -0400
+++ b/lib/galaxy/web/controllers/root.py Thu Oct 16 14:12:37 2008 -0400
@@ -219,26 +219,26 @@
             data.info  = p.info
             
             # The following for loop will save all metadata_spec items
-            for name, spec in data.datatype.metadata_spec.items():
+            for name, spec in data.metadata.spec.items():
                 if spec.get("readonly"):
                     continue
                 optional = p.get("is_"+name, None)
                 if optional and optional == 'true':
                     # optional element... == 'true' actually means it is NOT checked (and therefore ommitted)
-                    setattr(data.metadata,name,None)
+                    setattr(data.metadata, name, None)
                 else:
-                    setattr(data.metadata,name,spec.unwrap(p.get(name, None), p))
+                    setattr( data.metadata, name, spec.unwrap( p.get (name, None) ) )
 
             data.datatype.after_edit( data )
             trans.app.model.flush()
             return trans.show_ok_message( "Attributes updated", refresh_frames=['history'] )
         elif p.detect:
             # The user clicked the Auto-detect button on the 'Edit Attributes' form
-            for name, spec in data.datatype.metadata_spec.items():
+            for name, spec in data.metadata.spec.items():
                 # We need to be careful about the attributes we are resetting
-                if name != 'name' and name != 'info' and name != 'dbkey':
+                if name not in [ 'name', 'info', 'dbkey' ]:
                     if spec.get( 'default' ):
-                        setattr( data.metadata,name,spec.unwrap( spec.get( 'default' ), spec ))
+                        setattr( data.metadata, name, spec.unwrap( spec.get( 'default' ) ) )
             data.datatype.set_meta( data )
             data.datatype.after_edit( data )
             trans.app.model.flush()
@@ -258,16 +258,12 @@
             # case it resorts to the old dbkey.  Setting the dbkey
             # sets it properly in the metadata
             data.metadata.dbkey = data.dbkey
-        metadata = list()
-        # a list of MetadataParemeters
-        for name, spec in data.datatype.metadata_spec.items():
-            if spec.visible:
-                metadata.append( spec.wrap( data.metadata.get(name), data ) )
         # let's not overwrite the imported datatypes module with the variable datatypes?
+        ### the built-in 'id' is overwritten in lots of places as well
         ldatatypes = [x for x in trans.app.datatypes_registry.datatypes_by_extension.iterkeys()]
         ldatatypes.sort()
         trans.log_event( "Opened edit view on dataset %s" % str(id) )
-        return trans.fill_template( "/dataset/edit_attributes.mako", data=data, metadata=metadata,
+        return trans.fill_template( "/dataset/edit_attributes.mako", data=data,
                                     datatypes=ldatatypes, err=None )
 
     @web.expose
diff -r 97a8f6f5ba00 -r a5831435c654 templates/dataset/edit_attributes.mako
--- a/templates/dataset/edit_attributes.mako Wed Oct 15 15:51:13 2008 -0400
+++ b/templates/dataset/edit_attributes.mako Thu Oct 16 14:12:37 2008 -0400
@@ -38,14 +38,14 @@
             </div>
             <div style="clear: both"></div>
           </div>
-          %for element in metadata:
-              %if element.display:
+          %for name, spec in data.metadata.spec.items():
+              %if spec.visible:
                 <div class="form-row">
                   <label>
-                      ${element.spec.desc}:
+                      ${spec.desc}:
                   </label>
                   <div style="float: left; width: 250px; margin-right: 10px;">
-                      ${element.get_html()}
+                      ${data.metadata.get_html_by_name( name )}
                   </div>
                   <div style="clear: both"></div>
                 </div>