Fix the processor instance caching

[MehmedGIT]

In #972, I missed using the context switch required for the cached processor instances to work correctly. Now I am able to reuse cached instances. According to my observations and testing, for both cached and non-cached, that additional line has no side effects.

[bertsky]

This is problematic:

1. obviously, you forgot the other branch of the conditional (with profiling enabled, there's also a processor.process()...)
2. your cached processors were all constructed with a non-null workspace argument, which causes them to chdir into that:
   

   core/ocrd/ocrd/processor/base.py

   Lines 140 to 142 in 9ae6ddf

   	if self.workspace:
   	self.old_pwd = getcwd()
   	os.chdir(self.workspace.directory)

3. your current workspace.directory might not be an absolute path; so it would become relative to the previous workspace, not to CWD of the caller

So either you rebase to the processor.old_pwd, or pass workspace=None when caching, or resolve to absolute here before calling.

(BTW, in my PR I also moved the chdir to run_api alone, which would be the equivalent of the second option I mentioned.)

[MehmedGIT]

@bertsky, thanks for the feedback, and insight. Indeed, I missed the first processor.process() when I skimmed through the code. It was a bit of rushed PR through the browser on Friday.

so it would become relative to the previous workspace, not to CWD of the caller

Oh. Why is this so? This feels a bit odd to what I would have expected - the CWD of the caller once the process() method exits. I will check in more detail how to get the desired behavior without changing the current logic much.

[bertsky]

so it would become relative to the previous workspace, not to CWD of the caller

Oh. Why is this so? This feels a bit odd to what I would have expected - the CWD of the caller once the process() method exits. I will check in more detail how to get the desired behavior without changing the current logic much.

Because your run_processor does change back to the old CWD, but the initial (constructor's) chdir does not get reverted ever after. (So "old CWD" could well become initial workspace at runtime, as opposed to caller's CWD.)

[bertsky]

Looks better now, but I still feel we should not pass workspace to get_processor at all.

(The processor constructor will chdir into that, so if it is a relative path, then our own subsequent chdir on L98 will not work.)

Better to omit the workspace kwarg (so None gets passed to the constructor), and then set processor.workspace = workspace afterwards (next to the chdir on L98).

[bertsky]

LGTM – thanks!

[MehmedGIT]

@kba, this should be good to be merged now. I will apply the same changes to #974 as well.

[tdoan2010]

Should we extensively use pushd_popd instead of try ... catch ... finally os.chdir() pattern?

[MehmedGIT]

Should we extensively use pushd_popd instead of try ... catch ... finally os.chdir() pattern?

Not without adapting pushd_popd first. However, I don't want to mess with that to not break something else.