search

igraph / rigraph

igraph R package
https://r.igraph.org
532 stars 200 forks source link

issues from CRAN checks #1419

Open maelle opened 3 days ago

maelle commented 3 days ago

https://cran.r-project.org/web/checks/check_results_igraph.html

maelle commented 3 days ago

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

maelle commented 3 days ago

@krlmlr

maelle commented 3 days ago

https://github.com/r-lib/cpp11/issues/355

maelle commented 3 days ago

https://github.com/r-lib/rlang/issues/1706

krlmlr commented 3 days ago

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

More context in https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.log .

Haven't managed to replicate with the Intel 2023.2 compiler from https://github.com/r-hub/containers/tree/main/containers/intel .

Same problem in duckdb.

szhorvat commented 3 days ago

https://www.stats.ox.ac.uk/pub/bdr/Intel/igraph.out

Here's the warning from the Intel compiler:

In file included from vendor/cigraph/src/misc/degree_sequence.cpp:27:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/algorithm:61:
In file included from /usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:61:
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_tempbuf.h:263:8: warning: 'get_temporary_buffer<vd_pair>' is deprecated [-Wdeprecated-declarations]
  263 |                 std::get_temporary_buffer<value_type>(_M_original_len));
      |                      ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:4996:15: note: in instantiation of member function 'std::_Temporary_buffer<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, vd_pair>::_Temporary_buffer' requested here
 4996 |       _TmpBuf __buf(__first, (__last - __first + 1) / 2);
      |               ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_algo.h:5070:23: note: in instantiation of function template specialization 'std::__stable_sort<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, __gnu_cxx::__ops::_Iter_comp_iter<bool (*)(const vd_pair &, const vd_pair &)>>' requested here
 5070 |       _GLIBCXX_STD_A::__stable_sort(__first, __last,
      |                       ^
vendor/cigraph/src/misc/degree_sequence.cpp:87:18: note: in instantiation of function template specialization 'std::stable_sort<__gnu_cxx::__normal_iterator<vd_pair *, std::vector<vd_pair>>, bool (*)(const vd_pair &, const vd_pair &)>' requested here
   87 |             std::stable_sort(vertices.begin(), vertices.end(), degree_less<vd_pair>);
      |                  ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/stl_tempbuf.h:99:5: note: 'get_temporary_buffer<vd_pair>' has been explicitly marked deprecated here
   99 |     _GLIBCXX17_DEPRECATED
      |     ^
/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/x86_64-redhat-linux/bits/c++config.h:2359:34: note: expanded from macro '_GLIBCXX17_DEPRECATED'
 2359 | # define _GLIBCXX17_DEPRECATED [[__deprecated__]]
      |                                  ^

As far as I can tell this is not our problem, but an issue with the combination of Intel CC and libstdc++ on that specific system. We use the std::stable_sort() function in a standard and non-deprecated way. The internal implementation of this function uses get_temporary_buffer, which is claimed to be deprecated, but we have no control over this. This is the internal implementation of stable_sort.

We normally compile the C core in C++11 mode. To double-check, I compiled in C++17 mode both with GCC 13 (libstdc++) and Clang 18 (libc++), and I see no warning.

Feel free to link to this message when submitting to CRAN. I'm happy to take another look if someone can point out a specific issue with the usage of stable_sort, but I don't believe there is any.

szhorvat commented 2 days ago

r-lib/cpp11#355

I believe the SETLENGTH calls comes from code distributed by cpp11, so all we need to do is to wait for that package to address the issue and recompile igraph.

szhorvat commented 2 days ago

r-lib/rlang#1706

The PREXPR call comes code included in igraph, which seems to have been borrowed from lazyeval. As I understand this was superseded by tidyeval then by rlang? Instead of including this code in igraph directly, can we use rlang, and let them fix this on their side?

maelle commented 1 day ago

@Antonov548 do you think some stuff should be fixed on igraph's side?

szhorvat commented 1 day ago

Yes, the PREXPR issue can only be fixed on igraph's side because it's in C code that's included in igraph (basically a vendored version of https://github.com/hadley/lazyeval). However, as I said above, it may be possible to strip this out and replace it with functionality from rlang. I don't know enough about R, and its different lazy evaluation features, to be able to tell if this is possible. But this requires R expertise, not C expertise.

maelle commented 1 day ago

could you please point me to one of the uses of PREXPR?

szhorvat commented 1 day ago

could you please point me to one of the uses of PREXPR?

Are you deeply familiar with R's C API (I'm definitely not)? If not, this is not what you should look at.

PREXPR is in src/lazyeval.c, which as I said is a vendored version of https://github.com/hadley/lazyeval

I suggest you look at what R functions R/lazyeval.R (the R counterpart of src/lazyeval.c) provides. See which of these are used and how they are used. See if these can be replaced with whatever the current rlang provides. What I was able to figure out some weeks ago is that this definitely won't be a drop-in replacement, as the lazy evaluation strategy is said to be different. But I don't know much R and I can't handle it. Do you think you can take a look @maelle ?

In particular, see the use of lazy_dots in make.R and lazy_dots / lazy_eval in iterators.R. Can these be replaced with some rlang features?

Antonov548 commented 1 day ago

PREXPR is in src/lazyeval.c, which as I said is a vendored version of https://github.com/hadley/lazyeval

I didn't see immediately your comment about vendoring of package. Good to know it's problem not only in R/igraph.

I also will take a look what could be alternatives in R's C API.

szhorvat commented 1 day ago

I also will take a look what could be alternatives in R's C API.

I don't think this is a good use of our time. See the issue I linked to (both in this thread and in the past). If the maintainers of packages like rlang think that this is exceedingly difficult, and can't come up with an immediate replacement, then I really don't think we should take this on.

At the risk of sounding like a broken record, I'll say it one last time: I think the only reasonable way forward is to transition to using rlang for lazy evaluation, which is an R programming task, not C. No more repetitive comments on this from me now, promise 😉

Antonov548 commented 1 day ago

I also will take a look what could be alternatives in R's C API.

I don't think this is a good use of our time. See the issue I linked to (both in this thread and in the past). If the maintainers of packages like rlang think that this is exceedingly difficult, and can't come up with an immediate replacement, then I really don't think we should take this on.

At the risk of sounding like a broken record, I'll say it one last time: I think the only reasonable way forward is to transition to using rlang for lazy evaluation, which is an R programming task, not C. No more repetitive comments on this from me now, promise 😉

Make sense. I also just not sure what can be used from rlang. Let's wait then oppnion from R experts.

maelle commented 23 hours ago

I am not sure when exactly but yes I know a bit of rlang, enough to look into this

mpadge commented 1 hour ago

@maelle FYI The "non-API calls" are definitely an issue from cpp11, and must be resolved there. It's affecting a lot of packages now. PR to fix has been open for a month ,alas.