Initial client script

[spigad]

Aim to wrap server negotiation, myproxy, upload of input. This ticket not include workflow creation which depend on CMSSW interaction (which has a dedicated ticket).
The script must implement :

• user code (default.tgz for those who are familiar with CRAB2) upload
• user cfg upload
• cmssw pset upload (config cache interaction)
• check status
• output retrieval

[spigad]

spiga: Replying to [ticket:207 spiga]:

Aim to wrap server negotiation, myproxy, upload of input

[cinquo]

mcinquil: Please Review

[cinquo]

mcinquil: This first version handles the cli interaction, the user configuration, the possibility to have more job type plugins, interact with the credentials and with the server.
The server interaction part is currently using httplib, but probably this part can be evaluated (anyway, in this implementation it is not being currently used for obvious reaons).
Current dependencies are:

• python2.6
• WMCore
• httplib2

[spigad]

spiga: I'd like to check in the code but Simon/Dave would comment/check first I think.

some clarifications I think may avoid confusion:

• Client is not depending on WMSPec and it will not do. WMSpec is only used at server level.. (currently in the REST interface built on ReqMgr..)
• Client deployment is a major topic to discuss.. at the moment there is bash script (a la crab2) which will help testing and we'll allow to sort out what we need for a more stable approach.
• Client is thin and must stay so. There is a structure in the package to allow versatility and flexibility... it has jobtype concept to cope at best with WMSpec philosophy.
• Client is expected to expose API
• Client is stateless
• Client has 2 expected behaviors :
  --- create a working dir per Request and put in logs and configs (I'm not expecting any need to have a structure in it). This will be the default at the beginning
  --- avoid working dir.
• Request names [which will correspond to the working dir if there] will be generated combining the name chosen by the user plus a UUID generated by the server. [TBD]

that should be more or less all for the moment.

[drsm79]

metson: Some comments from reading through the patch:

• I'd get rid of the specialised exceptions - just reuse ones that come from the standard library (e.g. the server should raise appropriate HTTP errors, which the client can report, that's not an exception though, it's a message in the log)
• I think loosing the jobtypes package entirely would be good. We can have the server validate that the required input is there (and raise an appropriate HTTP error if it's not). This is the sort of thing that the server needs to do anyway, and is the kind of thing that will make the thin client fat. (we could get clever in the future and have the server tell the client things to validate, e.g. required keys, but that's not needed now and might be more trouble than its worth)
• What is the plugins management for? If it's just job types this should go...
• I would move the optparse options out of the CrabClient class and into a standalone method that returns the opts, args and logger. You can then pass the logger around as needed to other classes.
• I think we should aim for the client to be a single python file, so I'd loose the package structure. If it becomes apparent that the client has to put on weight we can refactor things out into packages, but I'd start out with a single file (sooo much easier to install)
• I like the idea of using option groups - which groups would you have? I guess debugging, job management (submit/retrieve) and job monitoring?
• I think that {{{client command -h}}} is unnecessary - we should have a brief help that's visible by {{{client -h}}} and a link to twiki (or similar) documentation for more detailed explanationwa
• I'd name the client class something better than {{{obj}}} in the {{{main}}} method - e.g. client instance or similar.
• Is the dependency on WMCore for the configuration file really necessary? Could we use ini files (I can imagine a riot if we introduce a new format for the client configuration) or python dictionaries?
• For something that's meant to handle a load of HTTP traffic this doesn't yet :)
• Ditto for json
• More seriously, I'd like to see some of that in the client before the commit happens. I'd prefer to aim to avoid httplib2 (since it's not standard and we want a simple install). If we have to go with httplib2 (I don't think we do...) then we might want to revisit the WMCore dependency, and just reuse a bunch of code out of the Service package (cf. CMSCouch).
• We need to be careful about what we expose to users as configurables, things like the ConfigCache URL shouldn't be, IMHO, as that makes the management of servers much more complicated (e.g. you might have your configuration in a server that is down while talking to another one to submit your jobs, servers may not have the same access to cache instances as clients etc.) I'd like to see a list of what can be configured via client and what is set by the server somewhere (we did this for CRAB2, maybe it just needs a bit of a refresh)

[drsm79]

metson: Clarifications after chatting with Daniele re. JobType:

• Short term;
  • remove the Example jobtype (keep it around in documentation, but not in the code) and replace it with a real job type (Eric is working on one, I understand...)
  • (Just noticed this) change {{{validateConfig(self, config)}}} to {{{validateConfig(self)}}}
• Medium term; evaluate if other jobtypes are needed, try to minimise the number used into a few abstract classes (e.g. upload_config_and_input etc. rather than a 1:1 mapping with the Workflows/specs available in the server)
• Longer term; look into downloadable plugins from the server (probably another ticket is needed for that...)

[drsm79]

metson: Also, if the configuration file changes, a migration script should be provided...

[drsm79]

metson: So conclusion was:

• Remove exceptions and make other sytlistic changes
• Keep job types, but avoid proliferation of types, and aim for abstract types that provide the functionality needed for more than 1 workflow/spec
• Use WMCore configuration for now - to be evaluated later
• Use httplib not httplib2 for initial testing - caching (the main feature httplib2 offers) may not be needed/desirable. If it is we add it in, and go for a more WMCore based (slightly fatter) client, changing the code over should be simple (happy to help out), tests should also include curl in a while loop etc.
• The server should act as a proxy for ConfigCache, and this should not be exposed to the user as a configurable

I think that was all, Daniele can add whatever I've inevitably missed

[spigad]

spiga: About the deploy script we had a discussion too... and for completeness let me add as reference what Dave suggested time ago:

"On a related note to this, I started on a DeployWMClient script yesterday to allow users to deploy WMCore and httplib2 for config cache upload tests. Its probably a good idea to follow this method for Crab Client as well, either by adding it to the DeployWMClient stuff or cloning it to make a DeployCrabClient script (which I guess comes down to how many dependencies they share)

https://svnweb.cern.ch/trac/CMSDMWM/wiki/WMClientDeployment
"

[drsm79]

metson: Replying to [comment:10 spiga]:

About the deploy script we had a discussion too...

Ah yes, and I said I'd set up a sub system in the WMCore setup.py for CRAB Client (I'll do a server one too, while I'm at it...)

[drsm79]

metson: From Mattia:

• WMException on the client: I had a look and it seems to complex
  for the usage needed on the client (WMCore.WMException); I do not
  think on the client is needed to have something that prints out class,
  module, method, instance, line....of the exception; the only things I
  have been using them is to propagate errors to the main and to
  associate an exit code to the error (so from the CLI you have a real
  exit code of the client); anyway, even if we are going to use
  WMException, specific exceptions are still needed and may inherit from
  that (as other WMCore modules do: WorkQueueExceptions, CouchError,
  BossAirException, ResubmitterException);

The point is that the exceptions should in the majority of cases come from the server (e.g. if you can talk to the server and get a response any exception will be an HTTP error from the server). I don't think you need to wrap that further. Also, exceptions aren't that useful in the client - as you say, if one is raised it's basically going to print a message and exit, there's not going to be much beyond that in terms of exception handling - so having a bunch of specific exceptions is somewhat code bloat.

Basically I'd see the first version of the client have no exceptions (e.g. just use ones from pythons standard library. ValueError, HTTPError etc.) and only add specific exceptions in very complicated, rare cases. I wouldn't be surprised if the final client had no specialised exceptions in either...

• sorry, probably my lack: why is better to have the logger in a
  standalone file? in the class CrabClient interfaced by the CLI it is
  defined a logger (with level, stream and file handlers), which is
  appended to the root python logger; then each module just call that
  logger and uses it with the level-handlers already defined..

I think it's more a question of where loggers/options are handled. I'd argue that a CrabClient object should be given a root logger (which it either uses or makes it's own logger from) and it's configuration (determined from some options), and not get them itself. This means you can:

• change the implementation of how the logger/options are gathered
• unit test the class better

This basically results in something like the following client code:

{{{
def get_options_and_logger():
# do stuff, set the logger level based on user input
return opts, args, logger

if name == 'main':
opts, args, logger = get_options_and_logger()
# put the opts and args into some appropriate format, possibly just passing them on
client = CrabClient(configuration, logger)
# do stuff with the client
client.run_command(opts.command)
sys.exit(0)
}}}

But also means I can unit test the CrabClient class without any user input (e.g. by building a configuration for the class and passing in a bunch of commands for the run_command()).

Speaking of which... there's no unit tests for the classes, a first pass at them should be added before this goes in :)

[cinquo]

mcinquil: Please Review

[drsm79]

metson: Please Review

[drsm79]

metson: The attached patch should apply on top of Mattia's. It's not quite as finished as I'd like but I think shows some of what I was trying to get across earlier. I think the following dependencies from WMCore should be added (which makes install more complex than I'd like...):

• Configuration
• Proxy
• WMCore.Services.JSONRequests (which means httplib2)
• WMFactory

Anyway, give it a review and we'll go from there. Sorry to be a bit of a pain with all this, but I think it's important to start as we mean to go on.

[spigad]

spiga: Replying to [comment:15 metson]:

The attached patch should apply on top of Mattia's. It's not quite as finished as I'd like but I think shows some of what I was trying to get across earlier. I think the following dependencies from WMCore should be added (which makes install more complex than I'd like...):

• Configuration
• Proxy
• WMCore.Services.JSONRequests (which means httplib2)
• WMFactory

uhm... I see you changed your mind a bit after our chat.. All seems reasonable but this patch requires some time/effort for a version 0. I understood we agreed to go toward this model in 2 steps.. and I'm still convinced of this approach.

Anyway, give it a review and we'll go from there. Sorry to be a bit of a pain with all this, but I think it's important to start as we mean to go on.

If I'm allowed to stress one more time what I think I'd definitely go for the previous patch and use it as starting point.

Next milestones will then include the reviewed client based on what you are proposing and Mattia will start taking care of this ~right now.

anyway I leave you guys the very last word.

[evansde77]

evansde: Had a detailed look through both of the above patches, notes:

Does it really need that many directories? Surely these are modules at most? The goal is to keep the client simple, so starting with a boatload of dirs is probably a bad first step.

etc/ConfigTest.py => name something more obvious (CrabClientConfig.py?)

Documentation of modules and methods needs to be added. Anyone vaguely enterprising will look at the client to see how it works, make sure the docs & comments are clear from the start.

Dont see the need for the jobtype at all. Different types of job are handled on the reqmgr side, on the client side it would just be a string that would default to the Analysis std spec.

Dislike the client class in the init module, not obvious where to find it. If we are going to have a package at all, it should contain clearly named modules.

In the client class stuff like: def set_logger(self, logger) should be made into properties, it will streamline the code that much more.

I think most of the commands talking to the server will result in some sort of JSON returned, would suggest adding a simple formatter to print a JSON structure for command line summaries.
Most of the calls to the server should basically end up as sys.stdout.write( formatOutput ( callToServer(args) ) )

Just out of interest, if we are aiming for a simple client in the longer term, why dont we have a simple prototype to start with?
It would be very instructive to see the client operations reduced to simple functions to start with.

[spigad]

spiga: uhm... so what you agreed yesterday is going to change...

for what concern the jobtype I disagree.. what you say is an oversimplification... and it is not corresponding to what I learnt and what I have in mind. What I think for a client is something like a tool which should also help the final user to address his activities.. so jobtype is something which should wrap actions etc. and produce a well standardized output. Then the fact that with a string you can materialize a schema (getting it from the server) to be applied to a abstract class is another step ahead I already discussed with simon and it make perfectly sense to me.

for all the other comments (naming, documentation, logger etc) I think I see all useful tickets for the next version 1.

About your view of the commands I think I agree even I'd say you could over simplifying a bit the real life. At least this is my feeling with a +n coming from the discussion we previously had with "the crab side of the team". But for sure I'm open to evaluate this on the field.

answering to your last question I'd say that

• having a version 0 which is ready to go

• having people which need to contribute now on this land being comfortable with this version 0

• having 2 patch which are not changing dir structure but differs on other part and no one really address the "one/two files model"

  are three reasons to push something in right now.
  Also : combining this with the fact that we (I hope) have a clear understanding of where we must go for a version 1 allow me to say: Mattia please start working on version 1 following the well defined guideline while collecting and addressing feedback and lesson learnt during the first round of tests.

and if there is room for a question:
do you really see that this version 0 is currently closing the door for a "one/two file model"? if yes tell me where.. if not I'd one more time convinced that we should see / test and then decide.

Daniele

[evansde77]

evansde: Replying to [comment:18 spiga]:

uhm... so what you agreed yesterday is going to change...

for what concern the jobtype I disagree.. what you say is an oversimplification... and it is not corresponding to what I learnt and what I have in mind. What I think for a client is something like a tool which should also help the final user to address his activities.. so jobtype is something which should wrap actions etc. and produce a well standardized output. Then the fact that with a string you can materialize a schema (getting it from the server) to be applied to a abstract class is another step ahead I already discussed with simon and it make perfectly sense to me.

for all the other comments (naming, documentation, logger etc) I think I see all useful tickets for the next version 1.

About your view of the commands I think I agree even I'd say you could over simplifying a bit the real life. At least this is my feeling with a +n coming from the discussion we previously had with "the crab side of the team". But for sure I'm open to evaluate this on the field.

answering to your last question I'd say that

• having a version 0 which is ready to go

• having people which need to contribute now on this land being comfortable with this version 0

• having 2 patch which are not changing dir structure but differs on other part and no one really address the "one/two files model"

  are three reasons to push something in right now.
  Also : combining this with the fact that we (I hope) have a clear understanding of where we must go for a version 1 allow me to say: Mattia please start working on version 1 following the well defined guideline while collecting and addressing feedback and lesson learnt during the first round of tests.

and if there is room for a question:
do you really see that this version 0 is currently closing the door for a "one/two file model"? if yes tell me where.. if not I'd one more time convinced that we should see / test and then decide.

Daniele

I agreed yesterday that starting with a functional prototype and simplifying it down is a good way to go. I still feel that is a good way to proceed.
Then I looked at the code that Mattia and Simon have produced and critiqued it. I think its the wrong way to start assembling the client if you want to squeeze it down in the long run. It also looks like instead of starting with some well defined basics and testing those and adding to it, there is an attempt to go all singing & all dancing right from the off.

I dont really agree with you on the job type thing, I think it should be kept simple (simple, when dealing with users is a good thing) IMO jobtype = string which defines a dictionary of fields which you fill and post to the server.
Specifically the "tool which should also help the final user to address his activities" is the server. The client just pulls information from that.

I would say that if you have a decent idea of what version 2 is going to look like, version 0 & 1 should at least look like steps on the way to that.

In answer to your question, I dont think this looks like a version 0.
I would say version zero would consist of maybe 3 or four scripts.

1. module containing the auth/http request/json formatter
2. client_new_task.py - self contained script that pushes a new task to the server, configuration just being a dict or something in the module
3. client_status.py - pulls status for the tasks and prints it
4. client_remove.py - remove a task from the server.

Then you have a basic client that completes the fundamental thing that the client needs to do and you will see where the overlap is between the essentially 3 functions. You can test this, make sure its robust and then add other features once it has been proven. If you have to make changes, which you will for any prototype, its minimal invested effort, as opposed to full scale package refactors.

[spigad]

spiga: ok we can easily conclude that we have different points of view and we can discuss on concepts over and over again but honestly I don't see how to solve this... There are different approaches and disagreements which I feel are for principle by meaning wo/ numbers on the hands.

I feel committed to provide the support for the analysis with the agent. I'm trying to do that and I know that the client which has been proposed is not orthogonal to any basic concept.. It implements what I now expect for a client based on my experience and I feel like I spent some time supporting analysis in cms and I'd like to being able to express an opinion. So again it seems to me that many points on the discussions which have been raised are matter of points of view.
Said that if you think that I'm assuming wrong, and so that I'm not committed to enable the Agent to support the analysis please tell me asap..

I'm always happy to be reviewed by whom I know has a better and deep knowledge than me (as I usually do).. to be sure that I'm not writing bad code but there is an important concept: I would be also "free", by meaning being able to express my idea and knowledge coding what I feel committed too (please go to 1 ...and remark if I'm assuming wrong).

Said that I need you clarify which are the degree of freedom that people working in WM can have since I guess i did wrong assumptions...

Daniele

[evansde77]

evansde: Yep, code review is all about points of view. Ill let you decide on wether you want to listen to mine above, its you that gets to support & maintain it etc.

I would expect that the majority of the code for the client should exist in the unittests for the server API, since thats where you would have to exercise all the API's that the server will provide anyways.

[drsm79]

metson: Replying to [comment:16 spiga]:

Replying to [comment:15 metson]:

The attached patch should apply on top of Mattia's. It's not quite as finished as I'd like but I think shows some of what I was trying to get across earlier. I think the following dependencies from WMCore should be added (which makes install more complex than I'd like...):

• Configuration
• Proxy
• WMCore.Services.JSONRequests (which means httplib2)
• WMFactory

uhm... I see you changed your mind a bit after our chat.. All seems reasonable but this patch requires some time/effort for a version 0. I understood we agreed to go toward this model in 2 steps.. and I'm still convinced of this approach.

Sorry, I wasn't clear. I was listing the dependencies that the client has on WMCore - the first two come from Mattia's original patch, the last two I (begrudgingly) think should be added (as the ServerInteractions class is basically the JSONRequest, the plugin management does the same thing as WMFactory). I've not added the last two dependencies but I think it probably makes sense to. I've not changed my mind in that I think this is a bigger dependency than I'd like, but I think it's probably the only way to reduce code duplication.

The changes I made in the patch were, I think, all that we agreed upon. I've made some tweaks in response to what Dave mentioned, which I think tidies the code up a bit.

The lack of any unit tests in Mattia's patch (and the pretty poor test coverage of what I added) should be considered a blocker for this going into SVN IMHO.

[ericvaandering]

ewv: Where does this stand? I'd hoped that this would finally be allowed into SVN so that I could start working on some of the unit tests. Alternatively, I tried to apply the three patches here to get something to work with, but that doesn't seem to work either. Someone have instructions?

It seems to me that process is getting the way of work here and I don't really understand why.

[cinquo]

mcinquil: I will post a patch based on last Simon patch (and of course on top of the other patches).

[cinquo]

mcinquil: Replying to [comment:24 mcinquil]:

I will post a patch based on last Simon patch (and of course on top of the other patches).

...and after this patch the client should work fine, and unit tests dev. can start from the very basic tests Simon has already added.

[cinquo]

mcinquil: Please Review

[drsm79]

metson: * I think exposing tracebacks to the user isn't a good idea

• Like the better handling of importing the configuration
• Why remove the -v/--verbose option? Even if it's only temporary?
• {{{logging.Formatter(' %(message)s')}}} is insufficient for a useful log message - you need at least the timestamp and level in there (ditching the %(name)s is probably valid
• Why switch {{{'Help for the "%s" command' % command}}} to {{{'Help for the ' + command + ' command'}}}? The substitution is safer than catting up strings.
• I think a lot of the line breaking is unnecessary in the code, I find
  {{{
  self.commands['jobtypes'] = {'function': self.jobtype_info, 'options': []}

}}}
easier to read than
{{{
self.commands['jobtypes'] = {'function': self.jobtypeInfo,
'options': []}
}}}
and the first is still under the line limit we set in our pylintrc.

• The change to {{{jobtypeInfo}}} should be reverted:
  • It should use print not the logger.debug - it's meant to print out what types are supported
  • The arguments to the command should be the same as other commands
  • Probably this should be moved into {{{clientcommand}}} as a standalone function
• {{{loadConfig}}} should only load the config, the command function should do other operations (e.g. call {{{createWorkArea}}} for submit etc.), this means the client handler class can be pretty static, and adding new or modifying commands just happens in the {{{clientcommand}}} functions

Other wise looks good. Looks like the test dir didn't get added to my original patch, I shouldn't submit patches at 3am, I know you've got it from Dropbox - can you add it to your patch so Eric has something to work from?

[cinquo]

mcinquil: Replying to [comment:27 metson]:

• I think exposing tracebacks to the user isn't a good idea

I think before going to users will need some testing and bugs discovery...if a not expected traceback is raised me, Daniele, Eric, whoever want can easily see what's happened. Then, for user this wont be in there!

• Like the better handling of importing the configuration
• Why remove the -v/--verbose option? Even if it's only temporary?

Because, by default you have been setting logging level to WARNING and verbose was corresponding to INFO...while I would like to have INFO by default and verbose something else, but I think we shuld evaluate it.

• {{{logging.Formatter(' %(message)s')}}} is insufficient for a useful log message - you need at least the timestamp and level in there (ditching the %(name)s is probably valid

That is for user output, in the directory of the task you have the 'crab.log' which has more information (as the timestamp). I wouldn't expose to the user the file/class/method name, even in debug mode (since the client is thin and we expect to stay like this, for a developer/operator should be easy to understand the proble if any is in there).

• Why switch {{{'Help for the "%s" command' % command}}} to {{{'Help for the ' + command + ' command'}}}? The substitution is safer than catting up strings.
• I think a lot of the line breaking is unnecessary in the code, I find
  {{{
  self.commands['jobtypes'] = {'function': self.jobtype_info, 'options': []}

}}}
easier to read than
{{{
self.commands['jobtypes'] = {'function': self.jobtypeInfo,
'options': []}
}}}
and the first is still under the line limit we set in our pylintrc.

The full line is 83 chars. Which was the limit? Wasn't 80?
How to read I think is a point of view :)
I read better a parameter per row than all parameter in the line.

• The change to {{{jobtypeInfo}}} should be reverted:
  • It should use print not the logger.debug - it's meant to print out what types are supported

If you look it is only a print saying ~nothing. Anyway, I understand and I will remove it.

• The arguments to the command should be the same as other commands
• Probably this should be moved into {{{clientcommand}}} as a standalone function

Yes, I agree...than, should it preserve the print or use the logging as the other commands?

• {{{loadConfig}}} should only load the config, the command function should do other operations (e.g. call {{{createWorkArea}}} for submit etc.), this means the client handler class can be pretty static, and adding new or modifying commands just happens in the {{{clientcommand}}} functions

The point for this is that, the workarea should be created as soon as possible, as the file logger (which IMO should be per request, so inside the workarea). Than, I think that the need is that a workarea is always needed, unless the user will not specify an option saying "--nocache".
But, I will evaluate if createWorkArea would be more useful in call.

More in general, I do not think the configuration needs always to be checked in the loadConfig module (eg: thinking to an user that hit "crab status").

Other wise looks good. Looks like the test dir didn't get added to my original patch, I shouldn't submit patches at 3am, I know you've got it from Dropbox - can you add it to your patch so Eric has something to work from?

Ok. I forgot to add it as well :)

I will update the patch.

[drsm79]

metson: Replying to [comment:28 mcinquil]:

Replying to [comment:27 metson]:
I think before going to users will need some testing and bugs discovery...if a not expected traceback is raised me, Daniele, Eric, whoever want can easily see what's happened. Then, for user this wont be in there!

Right, but you could run with {{{--debug}}} and use a logger.debug - it's the print that I'm objecting to more than anything.

• Like the better handling of importing the configuration
• Why remove the -v/--verbose option? Even if it's only temporary?

Because, by default you have been setting logging level to WARNING and verbose was corresponding to INFO...while I would like to have INFO by default and verbose something else, but I think we shuld evaluate it.

I'd leave the -v flag in and make the default True, for now, then.

• {{{logging.Formatter(' %(message)s')}}} is insufficient for a useful log message - you need at least the timestamp and level in there (ditching the %(name)s is probably valid

That is for user output, in the directory of the task you have the 'crab.log' which has more information (as the timestamp). I wouldn't expose to the user the file/class/method name, even in debug mode (since the client is thin and we expect to stay like this, for a developer/operator should be easy to understand the proble if any is in there).

Even for user output having the timestamps and level are useful IMHO.

The full line is 83 chars. Which was the limit? Wasn't 80?
How to read I think is a point of view :)
I read better a parameter per row than all parameter in the line.

That's fine by me, either one per line or one line (but be consistent). I forget the line length we set but I think it was upped to 120 or so since everyone has nice widescreen mac's ;)

• The change to {{{jobtypeInfo}}} should be reverted:
  • It should use print not the logger.debug - it's meant to print out what types are supported

If you look it is only a print saying ~nothing. Anyway, I understand and I will remove it.

Right, it says nothing because there's no job types to print out yet...

• The arguments to the command should be the same as other commands
• Probably this should be moved into {{{clientcommand}}} as a standalone function

Yes, I agree...than, should it preserve the print or use the logging as the other commands?

Use print for this one (and probably for other places where getting information to the screen is the aim) - the point is that the result of the command is the list of job types being printed to the screen. If you use the logger your client options can hide the result of the command, which is not what you want.

• {{{loadConfig}}} should only load the config, the command function should do other operations (e.g. call {{{createWorkArea}}} for submit etc.), this means the client handler class can be pretty static, and adding new or modifying commands just happens in the {{{clientcommand}}} functions

The point for this is that, the workarea should be created as soon as possible, as the file logger (which IMO should be per request, so inside the workarea). Than, I think that the need is that a workarea is always needed, unless the user will not specify an option saying "--nocache".
But, I will evaluate if createWorkArea would be more useful in call.

I would have the logging separate from the working area (I agree you need a log per workflow) just so people can delete working areas while still having a copy of what went on. That doesn't change the valid point you made - something needs to create an area to write the log (and set up a work area). I'd move that into initialise(), or maybe even further up into the {{{bin/crab}}} script (and do the configuration loading there and just pass in the configuration object to the handler)

More in general, I do not think the configuration needs always to be checked in the loadConfig module (eg: thinking to an user that hit "crab status").

The configuration holds the URL for the CRAB server, so anything that talks to the server is going to need to read the config. It might be that we're putting too much in one configuration file - maybe there's a need to have a config that's for the job (CMSSW, runs, lumis etc) and another that holds more static/infrastructural information (server URL etc.)

Other wise looks good. Looks like the test dir didn't get added to my original patch, I shouldn't submit patches at 3am, I know you've got it from Dropbox - can you add it to your patch so Eric has something to work from?

Ok. I forgot to add it as well :)

Great minds fail alike... ;)

I will update the patch.

Thanks

[cinquo]

mcinquil: Replying to [comment:29 metson]:

Replying to [comment:28 mcinquil]:

Replying to [comment:27 metson]:
I think before going to users will need some testing and bugs discovery...if a not expected traceback is raised me, Daniele, Eric, whoever want can easily see what's happened. Then, for user this wont be in there!

Right, but you could run with {{{--debug}}} and use a logger.debug - it's the print that I'm objecting to more than anything.

Ok..but you have already set that to use the print. If you prefer mixing loggin and print to me it is fine (I would perefer "if options.debug: print traceback")

• Like the better handling of importing the configuration
• Why remove the -v/--verbose option? Even if it's only temporary?

Because, by default you have been setting logging level to WARNING and verbose was corresponding to INFO...while I would like to have INFO by default and verbose something else, but I think we shuld evaluate it.

I'd leave the -v flag in and make the default True, for now, then.

ok

• {{{logging.Formatter(' %(message)s')}}} is insufficient for a useful log message - you need at least the timestamp and level in there (ditching the %(name)s is probably valid

That is for user output, in the directory of the task you have the 'crab.log' which has more information (as the timestamp). I wouldn't expose to the user the file/class/method name, even in debug mode (since the client is thin and we expect to stay like this, for a developer/operator should be easy to understand the proble if any is in there).

Even for user output having the timestamps and level are useful IMHO.

While the TS are in the log file, we can start with minimal information and see if it will be needed.

The full line is 83 chars. Which was the limit? Wasn't 80?
How to read I think is a point of view :)
I read better a parameter per row than all parameter in the line.

That's fine by me, either one per line or one line (but be consistent). I forget the line length we set but I think it was upped to 120 or so since everyone has nice widescreen mac's ;)

:)

• The arguments to the command should be the same as other commands
• Probably this should be moved into {{{clientcommand}}} as a standalone function

Yes, I agree...than, should it preserve the print or use the logging as the other commands?

Use print for this one (and probably for other places where getting information to the screen is the aim) - the point is that the result of the command is the list of job types being printed to the screen. If you use the logger your client options can hide the result of the command, which is not what you want.

I wouldn't mix around the code logging and print calls (maybe printing the same things...then we'll end up with a dedicated class that inherit logging and does this).

• {{{loadConfig}}} should only load the config, the command function should do other operations (e.g. call {{{createWorkArea}}} for submit etc.), this means the client handler class can be pretty static, and adding new or modifying commands just happens in the {{{clientcommand}}} functions

The point for this is that, the workarea should be created as soon as possible, as the file logger (which IMO should be per request, so inside the workarea). Than, I think that the need is that a workarea is always needed, unless the user will not specify an option saying "--nocache".
But, I will evaluate if createWorkArea would be more useful in call.

I would have the logging separate from the working area (I agree you need a log per workflow) just so people can delete working areas while still having a copy of what went on. That doesn't change the valid point you made - something needs to create an area to write the log (and set up a work area). I'd move that into initialise(), or maybe even further up into the {{{bin/crab}}} script (and do the configuration loading there and just pass in the configuration object to the handler)

I do not see this to be so trivial:

• you need to know the taskname to create a meaningful working directory and I do not expect the bin/crab of your implementation will be doing this (which may include to look inside to user configuration file, to read things like workarea, taskname, ecc..)
• then the workarea, requestname should be potentially know not just by CRABClient.Handler.

Anyway I will try to put the createWorkArea somewhere else that applies to your comment.

More in general, I do not think the configuration needs always to be checked in the loadConfig module (eg: thinking to an user that hit "crab status").

The configuration holds the URL for the CRAB server, so anything that talks to the server is going to need to read the config. It might be that we're putting too much in one configuration file - maybe there's a need to have a config that's for the job (CMSSW, runs, lumis etc) and another that holds more static/infrastructural information (server URL etc.)

I hope that the server host:port and similar will not be in the user configuration, so that part will be removed (and since we have a central point we can evaluate to use an alias for the host and to always use that).

[cinquo]

mcinquil: Please Review

[drsm79]

metson: Please Review

[drsm79]

metson: If you guys are happy with the attached patch I'm happy that the code gets committed.

[drsm79]

metson: Please Review

[cinquo]

mcinquil: Please Review

[cinquo]

mcinquil: Please Review

[drsm79]

metson: That addition on top of my series looks fine to me (sorry I missed those imports out). Lets get this committed!

[drsm79]

metson: (In 27efe9f) First pass at a client for CRAB3
Fixes #207 #1173

From: Simon Metson simonmetson@googlemail.com