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
Add classes LSFJobManager and SCARFLSFJobManager #503
Add classes LSFJobManager and SCARFLSFJobManager #503
Conversation
…ger_and_SCARFLSFJobManager Pull IRemoteJobManager and the factory
…ger_and_SCARFLSFJobManager Conflicts: Code/Mantid/Framework/CMakeLists.txt Sort out conflict with removal of MDEvents, re #11064
Jenkins, be nice and retest this please |
It seems that this is more or less fine, but we got a |
Jenkins, you can do it, retest this please |
Token(std::string &u, std::string &t) : m_url(u), m_token_str(t){}; | ||
std::string m_url; | ||
std::string m_token_str; | ||
}; |
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.
Is the const qualifier missing before std::string&?
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.
Yes. I'll fix it.
const std::string &body) const { | ||
InternetHelper session; | ||
|
||
std::string ContTypeName = "Content-Type"; |
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.
Is there a reason to create a non-const variable?
const std::string &filename) { | ||
// build file name as given in the request body | ||
std::string upName = filename; | ||
std::replace(upName.begin(), upName.end(), '\\', '/'); |
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.
Shouldn't Poco::Path be used here to avoid this sort of things?
I've pushed changes for many of your comments and some other improvements to the unit tests that Owen suggested in another PR. A few changes, including the Poco URI and Path points, are still missing. I'll let you know and remove the "in progress" label when this is finished. |
I think this is ready again and the builds seem to be going well, so I've removed the "in progress" label. |
…_SCARFLSFJobManager Add classes LSFJobManager and SCARFLSFJobManager
…WebServiceAPIJobManage_from_RemoteAlgorithms_andRemoteJobManager Conflicts: Code/Mantid/Framework/API/inc/MantidAPI/RemoteJobManagerFactory.h Code/Mantid/Framework/API/test/RemoteJobManagerFactoryTest.h Code/Mantid/Framework/RemoteJobManagers/CMakeLists.txt Sort out conflict in CMakeLists (files added in another PR, #503) and factory test, re #11392
This addresses #11064.
This PR (see #398, #469) is about refactoring and putting the LSF and SCARF remote job manager code where it should be, ready for the remote algorithms revamp #11126. There is a sibling PR coming after this one for the Mantid API web service job manager #11392. The unit tests check the input/ouput/error handling/transaction logic with simple mocked server responses.
Note that I put these new classes in
Framework/RemoteJobManagers
. There are more coming but I wouldn't expect many. The amount of classes/files is not a reason to addRemoteJobManagers
but I thought that it'd be better to keep distinct things in different places. Feedback on this is appreciated. They could also go with theRemoteAlgorithms
on evenAPI
.To test: