py3: numerous string fixes to sage.numerical.backends

[embray]

Trying to make progress on my backlog of Python 3 fixes I haven't made tickets for...

This one is slightly questionable in that there were many cpdef functions in this package that take char * arguments, and I changed them to take non-typed arguments (effectively str).

I'm not exactly sure what implication this has for the C interfaces that were exported for these methods, or if we care.

Component: python3

Author: Erik Bray

Branch: da920d5

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/24740

[sagetrac-git]

Commit: f08fb45

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f08fb45

py3: numerous string encode/decode fixes for sage.numerical.backends

[jdemeyer]

Replying to @embray:

This one is slightly questionable in that there were many cpdef functions in this package that take char * arguments, and I changed them to take non-typed arguments (effectively str).

They don't seem to be performance-critical methods, so I'm fine with it.

One potential problem for reviewing this ticket is that it involves several external packages, some of which are not even free to install.

[jdemeyer]

comment:4

This ticket makes me think of a general problem with str_to_bytes: I think you should always accept bytes for filenames (Python generally does that). So it might be a good idea to allow str_to_bytes to accept bytes input (I said this before but you refused that). What do you think?

[jdemeyer]

comment:5

In src/sage/numerical/backends/gurobi_backend.pyx I see a if name which should be if name is not None

[jdemeyer]

comment:6

Several "vertex" methods in glpk_graph_backend.pyx used a take a char* as argument. To me, it's not obvious to me that the canonical replacement is str (as opposed to bytes). In any case, it feels wrong to disallow bytes.

[embray]

comment:7

Replying to @jdemeyer:

This ticket makes me think of a general problem with str_to_bytes: I think you should always accept bytes for filenames (Python generally does that). So it might be a good idea to allow str_to_bytes to accept bytes input (I said this before but you refused that). What do you think?

That would actually be bad because it would break most cases which explicitly should not accept bytes. Specifically for filenames, it's better to just make a special case for accepting bytes. Though I don't think high-level file handling functions need accept bytes in most cases either unless there were an explicit reason to.

[embray]

comment:8

Replying to @jdemeyer:

Several "vertex" methods in glpk_graph_backend.pyx used a take a char* as argument. To me, it's not obvious to me that the canonical replacement is str (as opposed to bytes). In any case, it feels wrong to disallow bytes.

I can't say I'm 100% clear on that myself since it's not obvious that this code is even used anywhere in Sage. But it's duplicating the Sage Graph API and I don't see what makes you think it should accept bytes.

[embray]

comment:9

Replying to @jdemeyer:

Replying to @embray:

This one is slightly questionable in that there were many cpdef functions in this package that take char * arguments, and I changed them to take non-typed arguments (effectively str).

They don't seem to be performance-critical methods, so I'm fine with it.

One potential problem for reviewing this ticket is that it involves several external packages, some of which are not even free to install.

It is a problem, and some of these modules are not well tested because of it. Though in the cases I've changed it should be fairly clear that it's the right approach in general (i.e. we are passing python strs to interfaces that expect char *).

[embray]

comment:11

I believe this issue can reasonably be addressed for Sage 8.4.

[jdemeyer]

comment:12

Replying to @embray:

Specifically for filenames, it's better to just make a special case for accepting bytes.

How about a new function filename_to_bytes()? I never liked the look of str_to_bytes(fname, FS_ENCODING, 'surrogateescape') anyway, filename_to_bytes(fname) would look much better.

[embray]

comment:13

How does this ticket now relate to #26020? It wasn't clear to me why it needed to broken into a separate ticket in the first place.

Are we still going to add filename_to_bytes()? I like the idea but it should probably be done separately.

[mkoeppe]

comment:15

These changes look fine to me (but I haven't tested anything).

[fchapoton]

comment:17

@embray, will you be able to finish this one and #24741 ?

[embray]

comment:18

Sorry, yeah, I'll take care of it. I think I stalled on this one because Jeroen had made some comments earlier, which seemed to suggest a course of action he preferred, and I was waiting for confirmation from him but it never came. I'll just clean up the existing fixes here and worry about the filename_to_bytes() thing later.

[embray]

comment:21

I just rebased the existing branch on 8.5.beta6.

I can see that there's a case to be made for still allowing some of these methods to accept filenames as bytes, but I think this is fine for now until and unless a specific need for it arises.

[fchapoton]

comment:22

the import of char_to_str seem not to be used in backends/coin_backend.pyx

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

da920d5

py3: numerous string encode/decode fixes for sage.numerical.backends

[sagetrac-git]

Changed commit from 3434d49 to da920d5

[fchapoton]

Reviewer: Frédéric Chapoton

[fchapoton]

comment:25

in src/sage/numerical/backends/cvxopt_sdp_backend.pyx,
the INPUT field must be modified

same in src/sage/numerical/backends/generic_sdp_backend.pyx

otherwise, looks good to me (as far as I can tell)

[embray]

comment:26

Ah, very true (not that many people are likely to read that, but best to be consistent).

[embray]

comment:27

No, they're fine. I checked the code and it doesn't need updating in those cases. For some reason they already correctly read str instead of char *.

[fchapoton]

comment:28

oh, well. Let it be.

@vbraun: probably safer to keep this one for 8.6.beta0

[vbraun]

Changed branch from u/embray/python3/sage-numerical-backends/string to da920d5

[seblabbe]

comment:31

Maybe the problem described in ticket #26999 was produced by the numerous string fixes made here. Can you take a look?

[seblabbe]

Changed commit from da920d5 to none