Is this copy necessary in PytorchModel ?

[mspronesti]

Hello,
I'm using the torch-based classical model you implemented on a "not-so-toy" dataset of 15 000 sentences (around 15 words each). Using the CPU, it takes quite a while, using the GPU it takes even more. This made me think that for each batch you introduce some overhead which makes the CPU primarily involved in the computation. Monitoring the GPU usage, it was around 2-10% for most of the time required to do one epoch. Digging into the source code, I saw this line

lambeq/lambeq/training/pytorch_model.py

Line 136 in d911686

diagrams = pickle.loads(pickle.dumps(diagrams)) # deepcopy, but faster

being the diagrams very big, especially with "not-so-toy" datasets, doing a deepcopy (even if faster, as you claim) is very demanding in terms of resources and only involves the CPU. Since you never alter the diagrams you receive, I was wondering what is the reason why you want to copy them.

Commenting that line and reinstalling lambeq, on the very same usecase, I get

• x3 speedup wrt to the previous attempt
• same results (which is not surprising)
• 30 to 45% usage of the GPU for most of the epoch

However, before opening a pull-request I wanted to reach you guys, not to pollute your repository with possibly unwanted stuff and to make sure I'm not missing anything.
Cheers 😃 .

[Thommy257]

Hi @mspronesti,

thanks for brining this up, this is indeed something we need to fix! It is true that the copying of the diagram requires some extra resources. We need this operation because we destructively substitute the symbols (placeholder of tensors) with the concrete values:

lambeq/lambeq/training/pytorch_model.py

Lines 137 to 144 in d911686

	for diagram in diagrams:
	for b in diagram._boxes:
	if isinstance(b._data, Symbol):
	try:
	b._data = parameters[b._data]
	b._free_symbols = {}
	except KeyError:
	raise KeyError(f'Unknown symbol {b._data!r}.')

Deleting the line shouldn't change the behaviour of the model because PyTorch changes the parameters in-place during the training. However, the instance of the lambeq Dataset class won't be reusable after training. If this is not a problem for you, I'd say yes, commenting out line 136 should be save to do.

However, we'll need to find a proper solution for this in general.

[mspronesti]

Hi, thanks for you answer.
I didn't notice the substitution of the symbols, my bad. Given the structure of the class and the fact that, I guess, you want people to be able to use the model even without the Trainer class and its sub-classes, I guess one might consider only copying _data and _free_symbols , storing the value to return in a variable, restore those two and eventually return. Not sure it's doable, I don't see many alternatives to this. Thanks again for you quick reply !