[v6r17] Introduce meta-filters in TS #3180

arrabito · 2016-11-21T14:22:49Z

This PR is about the introduction of 'meta-filters' in the TS.
All the details are explained in the forum:
https://groups.google.com/forum/?hl=en#!topic/diracgrid-develop/BAQYOgLNCAI
As I explained in my last post, even if I already have in mind some improvements on the code, it should be safe to merge it, since the new functionality is optional. Moreover, it has been already successfully tested in the production setup of CTA. Also unit and integration tests pass.

arrabito · 2016-11-22T13:06:36Z

With the last commit I've moved the logic from the client to the server side as explained in the first point of the work plan:
https://groups.google.com/forum/?hl=en#!topic/diracgrid-develop/BAQYOgLNCAI

The integration test still works, but I've removed the unit tests introduced with the first commit, since they are not valid anymore.

coveralls · 2016-11-22T13:11:23Z

Coverage decreased (-0.02%) to 16.877% when pulling 46bd2e9 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

arrabito · 2016-11-23T11:29:39Z

Concerning the three other points of the work plan, detailed here:
https://groups.google.com/forum/?hl=en#!topic/diracgrid-develop/BAQYOgLNCAI
id est:
2. "Changing FileMetadata"
I agree that things should be kept as simple as possible. So I would say that we could keep the current implementation, where once a file is attached to a transformation, it remains attached to it, until the transformation is deleted.

"Add the possibility to create a transformation without any inputdataquery and to add an inputdataquery afterwards"
I've verified that it's already the case. Here below a working example:

from DIRAC.TransformationSystem.Client.Transformation import Transformation
transformation = Transformation()
MDdict1b = {'particle':'gamma_diffuse', 'zenith':{"<=": 20}}
mqJson1b = json.dumps( MDdict1b )
.....
transformation.setFileMask(mqJson1b)
transformation.addTransformation()

"Collect your suggestion about how avoiding multiple FileCatalog instantiations"
After having moved the logic of the methods with filters to the server side (TransformationDB), the FileCatalog is not instantiated anymore in the TransformationClient. However, it's instanciated in the TransformationDB, as it was already the case for instance for the addDirectory method.

So, in principle the workplan is completed. Let me know your comments.

chaen · 2016-11-24T08:58:02Z

Resources/Catalog/TSCatalogClient.py

-  WRITE_METHODS = FileCatalogClientBase.WRITE_METHODS + [ "addFile", "removeFile" ]
+  WRITE_METHODS = FileCatalogClientBase.WRITE_METHODS + [ "addFile", "removeFile", "setMetadata" ]
+
+  NO_LFN_METHODS = [ "setMetadata" ]


Why is setMetadata in NO_LFN_METHODS ? It takes an LFN as first argument no ?

I admit I'm not sure about the definition of 'NO_LFN_METHODS', I've put it here in analogy with the FileCatalogClient.

It is in the doc of FileCatalog:

The names of those methods are reported by the plug-ins as "no_lfn" methods in the getInterfaceMethods() call. For those methods there is obviously no additional check of the structure of the LFNs argument and no corresponding processing of the results ```

Ok, but anyway, we should be coherent between the different catalog plugins, shouldn't we?

You are both right. The setMetadata should be a normal "Write" method following LFN conventions. This is not like that right now in FileCatalogClient. I suggest to merge this PR as it is and in the next release review the metadata related methods consistently for all the catalogs. I will make an issue on that in order not to forget it.

chaen · 2016-11-24T10:39:38Z

TransformationSystem/DB/TransformationDB.py

    if addFiles and fileMask:
-      self.__addExistingFiles( transID, connection = connection )
+      mqDict = json.loads( fileMask )
+      res = catalog.findFilesByMetadata( mqDict )


You should test res['OK']

Done in commit: 36b3a10

chaen · 2016-11-24T10:41:02Z

TransformationSystem/DB/TransformationDB.py

+         # Add the files to the transformations
+        fileIDs = []
+        for lfn in filesToAdd:
+          if lfnFileIDs.has_key( lfn ):


if lfn in lfnFileIDs
Even better and much faster, make a set intersection between filesToAdd and lfnFileIDs

chaen · 2016-11-24T10:41:46Z

TransformationSystem/DB/TransformationDB.py

@@ -479,6 +494,8 @@ def __deleteTransformationParameters( self, transID, parameters = None, connecti

  def addFilesToTransformation( self, transName, lfns, connection = False ):
    """ Add a list of LFNs to the transformation directly """
+    gLogger.info( "TransformationDB.addFilesToTransformation: Attempting to add %s files." % lfns )


len(lfns) ?

chaen · 2016-11-24T10:42:29Z

TransformationSystem/DB/TransformationDB.py

+      res = catalog.getFileUserMetadata( lfn )
+      if not res['OK']:
+        failed[lfn] = res['Message']
+        return S_OK( {'Successful':successful, 'Failed':failed } )


Why do you return after the first failure ?

You're right. Changed with:
if not res['OK']:
gLogger.error( "Failed to getFileUserMetadata for file", "%s: %s" % ( lfn, res['Message'] ) )
failed[lfn] = res['Message']
continue

chaen · 2016-11-24T10:46:49Z

TransformationSystem/DB/TransformationDB.py

+    gLogger.info( "setMetadata: Attempting to set metadata %s to %s" % (usermetadatadict, path) )
+    successful = {}
+    failed = {}
+    if isinstance( path, dict ):


I am not sure what you are trying to achieve with this test ?

I found why I had introduced this test... I had some type mismatch problem, because I was using:
@checkCatalogArguments
with
def setMetadata( self, path, metadatadict ):
in Resources/Catalog/TSCatalogClient

So you are right, the test is not necessary.

chaen · 2016-11-24T10:47:20Z

TransformationSystem/DB/TransformationDB.py

+    filesToAdd = []
+
+    catalog = FileCatalog()
+    isFile = catalog.isFile( path )['Value']['Successful'][path]


Either test res['OK'] or use the .get syntax

chaen · 2016-11-24T10:48:07Z

TransformationSystem/DB/TransformationDB.py

+
+    if not res['OK']:
+      failed[path] = res['Message']
+      return S_OK( {'Successful':successful, 'Failed':failed } )


I think I understand now. You only take the first element of the directory, but still return it as a multi input structure ?

I'm not sure to understand your question. But one thing I have to check, is that I was assuming that the corresponding setMetadata method in the FileCatalogDB takes as input either a directory, either a single file, but not a list of files.

Yes, this is my question. But you only treat one element in our method, and return a dictionary of successful / failed

Yes, the method treats one element, as for the FileCatalogDB, which is either a file, either a directory. The reason why the method returns a dictionary of successful / failed is that, the effect of setMetadata in this case is to add to the transformations all the files that match the query conditions of the transformations. So the effect of setMetadata is on a set of files...
Does it make sense in this case to return a successful / failed dictionary?

I don't think so, because the Failed/Successful dict are indexed on the input path. Since there is a single one, I don't think it is worth having this extra level

OK, I've just changed it in commit:
9faad26
with some other minor changes.

chaen · 2016-11-24T10:48:26Z

TransformationSystem/DB/TransformationDB.py

+    metadatadict.update( usermetadatadict )
+    gLogger.info( 'Filter file with metadata:', metadatadict )
+    fileTrans = self._filterFileByMetadata( metadatadict )
+    if not ( fileTrans ):


The parenthesis are not needed

chaen · 2016-11-24T10:48:57Z

TransformationSystem/DB/TransformationDB.py

+      path = [path]
+    else:
+      res = catalog.findFilesByMetadata( metadatadict, path )
+      path = res['Value']


you should test res['OK']

chaen · 2016-11-24T10:49:06Z

TransformationSystem/DB/TransformationDB.py

+      successful[path] = False
+    elif isFile:
+      filesToAdd.append( path )
+      path = [path]


Why do you do that ?

chaen · 2016-11-24T10:49:23Z

TransformationSystem/DB/TransformationDB.py

+    # Add the files to the transformations
+    gLogger.info( 'Files to add to transformations:', filesToAdd )
+    if filesToAdd:
+      for transID, lfns in transFiles.items():


iteritems

chaen · 2016-11-24T10:49:39Z

TransformationSystem/DB/TransformationDB.py

+      for transID, lfns in transFiles.items():
+        res = self.addFilesToTransformation( transID, lfns )
+        if not res['OK']:
+          for lfn in lfns:


Why not returning res ?

chaen · 2016-11-24T10:50:29Z

TransformationSystem/DB/TransformationDB.py

+            failed[lfn] = res['Message']
+        else:
+          for lfn in lfns:
+            successful[lfn] = True


A speedup:
successful = dict.fromkeys(lfns, True)

chaen

I don't know about the logic of the implementation, but I just added a couple of coding suggestions

chaen · 2016-11-24T10:51:30Z

TransformationSystem/DB/TransformationDB.py

+    typeDict = res['Value']['FileMetaFields']
+    typeDict.update( res['Value']['DirectoryMetaFields'] )
+
+    for transID, query in queries:


queries.iteritems ? Or is queries a list of tuples ?

It's a list of tuples

chaen · 2016-11-24T10:51:55Z

TransformationSystem/DB/TransformationDB.py

+      gLogger.info( "Apply query %s to metadata %s" % ( mq.getMetaQuery(), metadatadict ) )
+      res = mq.applyQuery(metadatadict)
+      if not res['OK']:
+        gLogger.error( "Error in applying query: %s" % res['Message'] )


return res ?

Yes. Done in commit: 36b3a10.

chaen · 2016-11-24T10:52:32Z

TransformationSystem/DB/TransformationDB.py

+      res = mq.applyQuery(metadatadict)
+      if not res['OK']:
+        gLogger.error( "Error in applying query: %s" % res['Message'] )
+      elif res['Value'] == True:


== True is not needed

fstagni · 2016-11-25T16:14:28Z

tests/Integration/TransformationSystem/Test_Client_Transformation.py

+            CatalogURL = Transformation/TransformationManager
+            }
+
+        ...


Originally this test was supposed to be for the "TransformationSystem only". With the test that you are adding here it supposes instead that also the DFC is up and running. I think you should move this new test in a separate module (this would also simplify the management of CI infrastructure)

OK. I've put it in a separate module. Please check that the module name and the location are appropriate or suggest a new one.

fstagni · 2016-11-25T16:15:14Z

tests/Integration/TransformationSystem/Test_Client_Transformation.py

+    self.fc = FileCatalog()
+    self.dm = DataManager()
+    self.metaCatalog = 'DIRACFileCatalog'
+    gLogger.setLevel( 'INFO' )


Inside a test, please always use "DEBUG"

fstagni · 2016-11-25T16:19:13Z

TransformationSystem/DB/TransformationDB.py

@@ -479,6 +494,8 @@ def __deleteTransformationParameters( self, transID, parameters = None, connecti

  def addFilesToTransformation( self, transName, lfns, connection = False ):
    """ Add a list of LFNs to the transformation directly """
+    gLogger.info( "TransformationDB.addFilesToTransformation: Attempting to add %s files." % lfns )
+    gLogger.info( "TransformationDB.addFilesToTransformation: to Transformations: %s" % transName )


Maybe you want to merge the last 2 gLogger calls

coveralls · 2016-11-25T20:12:58Z

Coverage decreased (-0.04%) to 16.864% when pulling 7a51fc1 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

coveralls · 2016-11-28T16:53:26Z

Coverage decreased (-0.03%) to 16.871% when pulling 24b3b96 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

coveralls · 2016-11-28T16:56:16Z

Coverage decreased (-0.03%) to 16.871% when pulling 24b3b96 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

coveralls · 2016-11-28T17:19:05Z

Coverage decreased (-0.03%) to 16.871% when pulling 7bd8959 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

fstagni · 2016-11-28T17:30:11Z

IMHO this looks OK. I let Chris make the last comments, if any. I remove the "WIP".

coveralls · 2016-11-29T09:41:40Z

Coverage decreased (-0.03%) to 16.872% when pulling 9faad26 on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

fstagni · 2016-11-29T11:39:01Z

Ok Luisa I think that code-wise there are no more big changes to do. The only thing to add now would be documentation, still in this PR.

arrabito · 2016-11-29T12:15:51Z

OK, do you mean some documentation about how to use this functionality (and how it works) to be added in:
http://dirac.readthedocs.io/en/rel-v6r15/AdministratorGuide/Systems/Transformation/index.html
?

fstagni · 2016-11-29T14:30:33Z

Yes, for example.

arrabito · 2016-11-29T14:55:33Z

Ok. Just one question, I don't remember exactly why, but the doc at the url:
http://dirac.readthedocs.io/en/rel-v6r15/AdministratorGuide/Systems/Transformation/index.html
or even:
http://dirac.readthedocs.io/en/rel-v6r16/AdministratorGuide/Systems/Transformation/index.html
is slightly different from the one in the master branch in DIRACDocs github. Should I anyway start from the this last one?

fstagni · 2016-11-29T15:00:40Z

Always work on a release branch, not on the master branch. This is true also for documentation. I think at some point I made myself the mistake of modifying the documentation on the master branch, so now they diverge a little. If you can, try to synchronize at least this page. Anyway, for this case just keep adding to this PR.

coveralls · 2016-11-30T14:28:42Z

Coverage decreased (-0.03%) to 16.871% when pulling 41ff42c on arrabito:TSwithFilters into bf52c4c on DIRACGrid:rel-v6r17.

arrabito · 2016-11-30T14:59:55Z

OK. Thank you. I've added the new documentation and tried to syncronizing the two branches.

fstagni · 2016-11-30T17:17:37Z

Very good documentation. Review OK.

introduce meta-filters in TS

fa6cb85

arrabito mentioned this pull request Nov 21, 2016

[v6r17] Transformation System: Introduction of meta-filters #3175

Closed

arrabito changed the title ~~introduce meta-filters in TS~~ [v6r17] Introduce meta-filters in TS Nov 21, 2016

move the logic of methods with filters from client to server side

46bd2e9

chaen reviewed Nov 24, 2016

View reviewed changes

fstagni reviewed Nov 25, 2016

View reviewed changes

added some checks

7a51fc1

fstagni changed the title ~~[v6r17] Introduce meta-filters in TS~~ [WIP] [v6r17] Introduce meta-filters in TS Nov 28, 2016

arrabito added 2 commits November 28, 2016 17:43

move test for TS DFC Catalogs in a separate modules, plus minor fixes

36b3a10

change loglevel to DEBUG

24b3b96

minor change

7bd8959

fstagni changed the title ~~[WIP] [v6r17] Introduce meta-filters in TS~~ [v6r17] Introduce meta-filters in TS Nov 28, 2016

change return of setMetadata

9faad26

add documentation

41ff42c

fstagni approved these changes Nov 30, 2016

View reviewed changes

fstagni added the Review OK label Nov 30, 2016

atsareg merged commit e941862 into DIRACGrid:rel-v6r17 Nov 30, 2016

arrabito deleted the TSwithFilters branch August 8, 2017 15:14

[v6r17] Introduce meta-filters in TS #3180

[v6r17] Introduce meta-filters in TS #3180

Conversation

arrabito commented Nov 21, 2016

arrabito commented Nov 22, 2016

coveralls commented Nov 22, 2016

arrabito commented Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaen Nov 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 25, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

fstagni commented Nov 28, 2016

coveralls commented Nov 29, 2016

fstagni commented Nov 29, 2016

arrabito commented Nov 29, 2016

fstagni commented Nov 29, 2016

arrabito commented Nov 29, 2016

fstagni commented Nov 29, 2016 via email

coveralls commented Nov 30, 2016

arrabito commented Nov 30, 2016

fstagni commented Nov 30, 2016

arrabito commented Nov 23, 2016 •

edited

Loading

chaen Nov 25, 2016 •

edited

Loading